Bug 203187
| Summary: | [kernel-shark] Removing KS_* in C++ source, other packaging fixes | ||
|---|---|---|---|
| Product: | Tools | Reporter: | Troy Engel (tengel) |
| Component: | Trace-cmd/Kernelshark | Assignee: | Default virtual assignee for Trace-cmd and kernelshark (tools_tracecmd_kernelshark) |
| Status: | RESOLVED PATCH_ALREADY_AVAILABLE | ||
| Severity: | normal | CC: | howaboutsynergy, rostedt, ykaradzhov |
| Priority: | P1 | ||
| Hardware: | All | ||
| OS: | Linux | ||
| Kernel Version: | 5.0.3-arch1-1-ARCH | Subsystem: | |
| Regression: | No | Bisected commit-id: | |
| Attachments: | Implement QStandardPaths in KsMainWindow.cpp | ||
|
Description
Troy Engel
2019-04-09 00:32:40 UTC
Hi Troy, Sorry for the late reply. We didn't have the email preferences set up for bugzilla properly and we were not getting notifications. We just noticed this. Can you send this patch over to linux-trace-devel@vger.kernel.org and we can continue the conversation there? Thanks a lot for your efforts in maintaining trace-cmd / KernelShark in Arch Linux. Note, we are currently working to get libtraceevent packaged as a standalone library (currently lives both in trace-cmd.git and the Linux kernel source tree). We'll need help from all the distributions to get that released. -- Steve Hi Troy, First of all, thank you very much for your work! Please accept my apologize for the big delay of my answer. It looks like I didn't configure properly my Bugzilla account, so I haven't been receiving emails when a new bug report is opened. I hope that everything is OK now and I will start getting those emails. I guess this is just a coincidence, but are currently trying to fix some of the problems you reported. Please have a look in this patch-set that is currently being reviewed: https://lore.kernel.org/linux-trace-devel/20190423132741.17864-1-ykaradzhov@vmware.com/T/#t and those two patches that have been merged already: https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/?id=7094701b4c112944de3903b7e0984753c8b71fc2 https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/?id=a9de6fa3e218ac1c0dd60c60e0f822cba5b5bcde I think points 1, 2, 3, 6 and 8 in your report are being addressed by these patches. We will greatly appreciate if you can review/test the patches and let us know if there is something that is still wrong, or can be done in a better way. Your comment in points 4 and 5 is something we haven't think about so far. Always having the -g compile option, is something that has been requested to us, but I never realized that this will be a problem when making the packages. I guess we have to reconsider the usаgе of this option. Point 6.5 is in my ToDo list. The reason for keeping it like this is because the icon in the repo is added just for testing. We are currently in the process of selecting the official icon. So just to summarize: We know that the new KernelShark is not ready yet to became a package. This is the reason why it is still on version 0.98. Any help will be more than welcome. Once again thanks! Yordan Hi guys, no problem - stuff happens. :) My patch is not useful (as-is) based on Yordan's update with the upcoming patches in review and the two commits above. :) We're all on the same wavelength, I'll switch to commenting on the work I'm reading over. These are just my opinions, not a professional coder by any means. ==== For item 1, I really don't think this is the complete approach as a Linux systems guy (my day job) as it's not using the available QStandardPaths class provided by Qt to be XDG compliant, specifically this commit's _getCacheDir() and lastSessionFile() methods: https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/?id=7094701b4c112944de3903b7e0984753c8b71fc2 ref: https://doc.qt.io/qt-5/qstandardpaths.html I think those should be something more like: QString dir = QStandardPaths::writableLocation(QStandardPaths::AppConfigLocation); Which equates to $HOME/.config/kernelshark/ as this is not a cache file, it's a configuration file which stores the size of the window and such, as well as the last loaded file. However, if you guys really want to use the cache directory, then this would be: QString dir = QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation); This would keep us XDG compliant, not hardcode those off of $HOME and generally be more portable (not that this is going to run on macOS anytime soon :) ). https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.6.html (I believe the intent is to allow read-only $HOME to have an $XDG_* setup that points to a writable area like a USB thumb drive, such as might be the case when booting off a live CD or some other similar scenario - I don't do this myself so not 100% sure) ==== That said, as I read the 5 patches being reviewed I get the feeling that a subtle problem is we're mixing up two ideas into the same "config" file - there's the settings of kernelshark as an application which as a user are generic to all sessions, this type of JSON data: "MainWindow": [ 960, 565 ], "Splitter": [ 0, 0 ], ...which I expect to save on exit and load on start automagically, regardless (just like most other desktop apps). Those definitely belong in $HOME/.config/kernelshark/, not $HOME/.cache/kernelshark. $0.02 But then we have the lastsession configuration which is about the specific *.dat files being examined and worked on (filters, markers, task plots, etc.) which relates to that specific data. Just trying to help, it's a bit of a fuzzy problem and there are probably other solutions - these lastsessions could go into the cache directory? But I'm wondering why not leave them in the config directory, are they _really_ cache files? Hrm. ==== Minor nit reading the plugin fixes: almost everywhere we use '.../kernelshark/' as the directory part but when the plugins are defined, it's .../kshark/ - it's my minor opinion this should be "kernelshark" to remain consistent with the naming conventions used throughout code and filesystem. Or, change all the others to "kshark". :) Thanks! Sorry this got kinda long, the rest of the upcoming patches seem to read OK to me reading them through. :) Once they get a commit I'll bang on the package to reflect the latest updates to git - waiting for this 5-patch set to commit is the smart move so that I don't flood the end users with too many packaging updates too fast. Steve happy to help with libtraceevent package; I don't want to confuse that work with this issue, if you want we can pop a new BZ here as well to discuss details? Just add me to CC, we'll do the needful. Right now I'm only seeing the static libraries get created and not the shared, so stuff to talk about. (In reply to Troy Engel from comment #3) > My patch is not useful (as-is) based on Yordan's update with the upcoming > patches in review and the two commits above. :) We're all on the same > wavelength, I'll switch to commenting on the work I'm reading over. These > are just my opinions, not a professional coder by any means. And we are not professional package developers ;-) I'm a Kernel developer trying to do things "right" in user space. This is all new terrain for me. > > ==== > > For item 1, I really don't think this is the complete approach as a Linux > systems guy (my day job) as it's not using the available QStandardPaths > class provided by Qt to be XDG compliant, specifically this commit's > _getCacheDir() and lastSessionFile() methods: > https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/ > ?id=7094701b4c112944de3903b7e0984753c8b71fc2 > > ref: https://doc.qt.io/qt-5/qstandardpaths.html > > I think those should be something more like: > > QString dir = > QStandardPaths::writableLocation(QStandardPaths::AppConfigLocation); > > Which equates to $HOME/.config/kernelshark/ as this is not a cache file, > it's a configuration file which stores the size of the window and such, as > well as the last loaded file. However, if you guys really want to use the > cache directory, then this would be: > > QString dir = > QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation); > > This would keep us XDG compliant, not hardcode those off of $HOME and > generally be more portable (not that this is going to run on macOS anytime > soon :) ). > https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.6.html > > (I believe the intent is to allow read-only $HOME to have an $XDG_* setup > that points to a writable area like a USB thumb drive, such as might be the > case when booting off a live CD or some other similar scenario - I don't do > this myself so not 100% sure) Thanks for the feedback. This is all useful information! > > ==== > > That said, as I read the 5 patches being reviewed I get the feeling that a > subtle problem is we're mixing up two ideas into the same "config" file - > there's the settings of kernelshark as an application which as a user are > generic to all sessions, this type of JSON data: > > "MainWindow": [ > 960, > 565 > ], > "Splitter": [ > 0, > 0 > ], > > ...which I expect to save on exit and load on start automagically, > regardless (just like most other desktop apps). Those definitely belong in > $HOME/.config/kernelshark/, not $HOME/.cache/kernelshark. $0.02 > > But then we have the lastsession configuration which is about the specific > *.dat files being examined and worked on (filters, markers, task plots, > etc.) which relates to that specific data. Just trying to help, it's a bit > of a fuzzy problem and there are probably other solutions - these > lastsessions could go into the cache directory? But I'm wondering why not > leave them in the config directory, are they _really_ cache files? Hrm. We're confused ourselves, trying to figure out what is "cache" and what is "config". > > ==== > > Minor nit reading the plugin fixes: almost everywhere we use > '.../kernelshark/' as the directory part but when the plugins are defined, > it's .../kshark/ - it's my minor opinion this should be "kernelshark" to > remain consistent with the naming conventions used throughout code and > filesystem. Or, change all the others to "kshark". :) We started getting lazy typing all those key strokes. But I agree. We should stick to "kernelshark" and be consistent. I want to reserve "kshark" for if we have a command line kernelshark interface. > > > Thanks! Sorry this got kinda long, the rest of the upcoming patches seem to > read OK to me reading them through. :) Once they get a commit I'll bang on > the package to reflect the latest updates to git - waiting for this 5-patch > set to commit is the smart move so that I don't flood the end users with too > many packaging updates too fast. > > > Steve happy to help with libtraceevent package; I don't want to confuse that > work with this issue, if you want we can pop a new BZ here as well to > discuss details? Just add me to CC, we'll do the needful. Right now I'm only > seeing the static libraries get created and not the shared, so stuff to talk > about. We're still working out the API, which is why we haven't worked out a shared object file yet. As that requires a bit more version control to work. But we need to get there. Thanks for all this, it was really helpful. -- Steve (In reply to Troy Engel from comment #3) > Hi guys, no problem - stuff happens. :) > > My patch is not useful (as-is) based on Yordan's update with the upcoming > patches in review and the two commits above. :) We're all on the same > wavelength, I'll switch to commenting on the work I'm reading over. These > are just my opinions, not a professional coder by any means. > > ==== > > For item 1, I really don't think this is the complete approach as a Linux > systems guy (my day job) as it's not using the available QStandardPaths > class provided by Qt to be XDG compliant, specifically this commit's > _getCacheDir() and lastSessionFile() methods: > https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/commit/ > ?id=7094701b4c112944de3903b7e0984753c8b71fc2 > > ref: https://doc.qt.io/qt-5/qstandardpaths.html > > I think those should be something more like: > > QString dir = > QStandardPaths::writableLocation(QStandardPaths::AppConfigLocation); > I am sending a patch that implements this. > Which equates to $HOME/.config/kernelshark/ as this is not a cache file, > it's a configuration file which stores the size of the window and such, as > well as the last loaded file. However, if you guys really want to use the > cache directory, then this would be: > > QString dir = > QStandardPaths::writableLocation(QStandardPaths::GenericCacheLocation); > > This would keep us XDG compliant, not hardcode those off of $HOME and > generally be more portable (not that this is going to run on macOS anytime > soon :) ). > https://specifications.freedesktop.org/basedir-spec/basedir-spec-0.6.html > > (I believe the intent is to allow read-only $HOME to have an $XDG_* setup > that points to a writable area like a USB thumb drive, such as might be the > case when booting off a live CD or some other similar scenario - I don't do > this myself so not 100% sure) > > ==== > > That said, as I read the 5 patches being reviewed I get the feeling that a > subtle problem is we're mixing up two ideas into the same "config" file - > there's the settings of kernelshark as an application which as a user are > generic to all sessions, this type of JSON data: > > "MainWindow": [ > 960, > 565 > ], > "Splitter": [ > 0, > 0 > ], > > ...which I expect to save on exit and load on start automagically, > regardless (just like most other desktop apps). Those definitely belong in > $HOME/.config/kernelshark/, not $HOME/.cache/kernelshark. $0.02 > > But then we have the lastsession configuration which is about the specific > *.dat files being examined and worked on (filters, markers, task plots, > etc.) which relates to that specific data. Just trying to help, it's a bit > of a fuzzy problem and there are probably other solutions - these > lastsessions could go into the cache directory? But I'm wondering why not > leave them in the config directory, are they _really_ cache files? Hrm. > The idea here is that the user can save/cache the state of the GUI in a given moment (Export the session) into a JSON file. Later this cached session data can be used to Import the session and put the GUI in the exactly same state as when the session was exported. By "same state" I mean loaded data, filters, markers e.t.c. but also window and splitter sizes. Note that the size of the canvas (in pixels) used to plot the graphs is actually very important, because this determines how much details are going to be visualized. The "Last session" button is just a quick way to use the explained above and return into the state of the GUI before closing it. > ==== > > Minor nit reading the plugin fixes: almost everywhere we use > '.../kernelshark/' as the directory part but when the plugins are defined, > it's .../kshark/ - it's my minor opinion this should be "kernelshark" to > remain consistent with the naming conventions used throughout code and > filesystem. Or, change all the others to "kshark". :) > I am sending a patch for this as well. Once again, thank you very much for the help! It will be great if you can subscribe to the mailing list (linux-trace-devel@vger.kernel.org) and keep an eye on what we are doing. Patches, reviews or bug reports are more than welcome. Cheers, Yordan > > Thanks! Sorry this got kinda long, the rest of the upcoming patches seem to > read OK to me reading them through. :) Once they get a commit I'll bang on > the package to reflect the latest updates to git - waiting for this 5-patch > set to commit is the smart move so that I don't flood the end users with too > many packaging updates too fast. > > > Steve happy to help with libtraceevent package; I don't want to confuse that > work with this issue, if you want we can pop a new BZ here as well to > discuss details? Just add me to CC, we'll do the needful. Right now I'm only > seeing the static libraries get created and not the shared, so stuff to talk > about. Thank you gentlemen for all your work!
I've just pulled the code and worked through the latest, I think we still have one outstanding item - TRACECMD_BIN_DIR being set to the build path, not /usr/bin/ (sic) when that's the destination directory. The overall code is still building in debug mode which I'm just sort of accepting for now. :)
After commenting out all patches/seds/etc. in my build, I ended up with a final warning from my build system:
==> WARNING: Package contains reference to $srcdir
usr/lib/kernelshark/libkshark-gui.so.0.9.8
usr/lib/kernelshark/libkshark.so.0.9.8
usr/lib/kernelshark/plugins/plugin-sched_events.so
I therefore had to restore these two seds in order to disable the debug builds responsible for the above issue:
# pass down Release as build type to strip build directories
# from final binaries
sed -i.cip 's|\(-D_INSTALL_PREFIX=\)|-DCMAKE_BUILD_TYPE=Release \1|g' \
Makefile
# the gcc/g++ debug flag is enabled even when _DEBUG=0 which causes
# the build directory to get coded into the libraries
sed -i.dbg 's/-Wall -g/-Wall/g' CMakeLists.txt
That left me though with libkshark-gui still having an embedded path to my local build directory, like so:
KsCmakeDef.hpp:#define TRACECMD_BIN_DIR "/home/tengel/abs/trace-cmd-git/src/trace-cmd/tracecmd"
So I restored this sed as well to set my target to end up in the /usr/bin/ target where I wanted it:
# the build sets TRACECMD_BIN_DIR=(local build path) instead of /usr/bin/
sed -i.tb 's|@TRACECMD_BIN_DIR@|@_INSTALL_PREFIX@/bin|g' build/deff.h.cmake
All other patches remvoed and doing great, my new build script was committed here this morning combining all of the above: https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=trace-cmd-git
Much appreciated for your continued work.
Hi Troy, I'm a little confused. Is there more that needs to be done to make this a smoother ride for packaging KernelShark? Should I re-open this bug? Thanks! -- Steve Hi Troy, It is true that TRACECMD_BIN_DIR is set to the build path, but this should not be a problem, because is is used only if the user starts KernelShark from its build location (which is never the case when you use the package). See here: https://git.kernel.org/pub/scm/utils/trace-cmd/trace-cmd.git/tree/kernel-shark/src/KsCaptureDialog.cpp#n491 I guess the real problem is that that the name itself (TRACECMD_BIN_DIR) creates confusion. Thanks a lot! Yordan Hi sorry for the delay I only have limited time during the week, sneaking a comment in before work. :) The problem with the design is that the full path to my build directory (which is random for every user) is embedded in the final binary. This is a no-no for Arch and our build system warns about that - no finished binaries should have these paths embedded in them. (they're usually random paths in my home directory where I build code) - the debug flags by design embed the paths; I must therefore sed the makefiles to remove the debug flags, as well as pass CMake the RELEASE build type (as it defaults to debug it seems) to handle this aspect of ensuring the compiler does the right thing. I assume eventually this will be resolved, building/dev in debug mode right now is the norm and OK, expected. (item #4 in my original report) - the path for TRACECMD_BIN_DIR is getting hard-coded into the final binary as it's a coding/design decision to do so. It's not the name of the variable (well, #define) and there's no confusion witht he name - it's simply that this technique causes a hard-coded build path into a binary that is a no-no and it has to get resolved. I am resolving it by changing your code to make it simply point to $PREFIX/bin (/usr/bin) to match the final location. (item #7 above) Rule: there can be no build paths embedded in the final binaries. Problem: kernelshark is embedding build directories in the final binaries due to the above two items, so I am resolving them with some sed work pre-build. Hope this helps clarify, thanks! (carrying over from our emails) We are testing these patches: https://patchwork.kernel.org/patch/10987563/ https://patchwork.kernel.org/patch/10987565/ They work, I get one minor fuzz due to code movement (my sed) but it's OK and applies cleanly: patching file kernel-shark/build/FindTraceCmd.cmake patching file kernel-shark/build/deff.h.cmake patching file kernel-shark/src/KsCaptureDialog.cpp patching file kernel-shark/CMakeLists.txt Hunk #2 succeeded at 57 with fuzz 2 (offset -4 lines). patching file kernel-shark/README The generic build still results in a debug build when used as-is; I needed to used this sed again: sed -i.cip 's|\(-D_INSTALL_PREFIX=\)|-DCMAKE_BUILD_TYPE=Release \1|g' \ Makefile This is something I just sort of Googled around and figured out based on trial and error; if `CMAKE_BUILD_TYPE=Release` is not passed down from the Makefile to cmake, the result _appears_ to be a Debug build as an internal fallback. In non sed form, what I'm doing is going from this: $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt $(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -D_INSTALL_PREFIX=$(prefix) .. to this: $(kshark-dir)/build/Makefile: $(kshark-dir)/CMakeLists.txt $(Q) cd $(kshark-dir)/build && $(CMAKE_COMMAND) -DCMAKE_BUILD_TYPE=Release -D_INSTALL_PREFIX=$(prefix) .. This is a hack just to get the job done; the README update sort of does the reverse and says "if you want a Debug, add -DCMAKE_BUILD_TYPE=Debug" but that appears to be what the default already is, so I have to reverse it and name it Release. But this isn't exactly true, it appears that if you don't name it at all, it's actually an empty string and does... something.. internally. There's a thread on it and even after reading I don't seem to understand what exactly the internal defaults are: http://cmake.3232098.n2.nabble.com/What-is-the-default-build-type-td7595975.html Most Google hits seems to end up with the same recommendation -- in your CMakeLists.txt, add a hook to trap it if not defined, and define it yourself. Basically: https://blog.kitware.com/cmake-and-the-default-build-type/ Is this right? I have no idea, I learn cmake as I go. :) But I know defining it with a quick sed results in the expected end results (no build paths in the binaries)... Hi Troy, Thanks a lot for helping us with this! Looking into the links in your post above I understood that my idea how to use CMAKE_BUILD_TYPE was wrong. I sent a new version of the patches: https://patchwork.kernel.org/project/linux-trace-devel/list/?series=132531 With the new patches you can still use -DCMAKE_BUILD_TYPE=Release but I also added -DCMAKE_BUILD_TYPE=Package and if you use this type you are free to make your own configuration of compiler flags. This can work like this cmake -DCMAKE_BUILD_TYPE=Package -DCMAKE_C_FLAGS_PACKAGE="-O3 -pedantic" -DCMAKE_CXX_FLAGS_PACKAGE="-O3 -other -flags" ../ Also I personally find more useful to build trace-cmd and KernelShark independently. Instead of modifying the Makefile you can first build only trace-cmd and then simply enter in kernel-shark/build/ and build and install the GUI following the kernel-shark/README cheers, Yordan Hi Yordan,
I don't feel there was a need for a whole new build type Package, I thought the answer would have been pretty simple to CMakeLists.txt:
if (NOT CMAKE_BUILD_TYPE)
set(CMAKE_BUILD_TYPE Release)
endif (NOT CMAKE_BUILD_TYPE)
This patchset is a 180 degree change in the other direction, making Debug builds the intentional default. I'll see what I can do with it when I get time later as it'll require some thought how to handle your new design choice, it should be possible to just use the Release build type here.
Hi, after sleeping on this I realize this is a design choice and I really have no right to question it, this is your project. The C++ patches fix the code-based problems which were really what was at stake, I'll react on the outside accordingly to the final cmake design once it's committed. And to be honest, I'm having bugzilla fatigue at this stage and need to step away. Thank you very much for all the work that got us this far, it will benefit the greater community as the new codebase rolls out to the distros. |