Bug 203187

Summary: [kernel-shark] Removing KS_* in C++ source, other packaging fixes
Product: Tools Reporter: Troy Engel (tengel)
Component: Trace-cmd/KernelsharkAssignee: 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
Created attachment 282183 [details]
Implement QStandardPaths in KsMainWindow.cpp

Hi Tzvetomir and Steven,

I'm approaching this as a more conversation to start, as the way I've identified and fixed some issues may not be how y'all want to go about it. Big picture, I currently maintain the Arch packages for trace-cmd/kernelshark and am working out the new Qt based kernelshark needs.

  https://aur.archlinux.org/packages/trace-cmd-git/

The recent commit https://git.kernel.org/pub/scm/linux/kernel/git/rostedt/trace-cmd.git/commit/?id=b9db93c51a4f21df7c01077bf3151944d104ee1b helped fix a lot of the hard-coded /usr/local problems, allowing me to remove a bunch of patchwork. What's below is where we're at as of commit g5276e83  (2019-04-08 19:00 CDT)


===

1. The largest one - I wrote a patch to implement the use of QStandardPaths for storing and loading the user configuration in KsMainWindow.cpp. Additionally, I implemented the use of (QStandardPaths::AppConfigLocation) in doing so, which according to the docs sets the minimum Qt to 5.5+ which may or may not be acceptable to this project. I'm attaching 001_ksdir_ksmainwindow.patch with this report as it was bigger than a quick sed. :)

===

2. Along the lines of #1, I have a very basic sed work to implement QDir::homePath() in KsCaptureDialog.cpp for loading/saving capture files, the use of KS_DIR is also problematic here (it's the random build directory from cmake)

  # this tries to use KS_DIR for open/close of files
  sed -i.hm 's/KS_DIR/QDir::homePath()/g' src/KsCaptureDialog.cpp

===

3. I think this is a bug - KsUtils.cpp looks for the kshark plugins by a different name then they are compiled/installed as, so it just fixes that source real quick to match the other stuff:

  # this source is hard-coded to KS_DIR/lib/plugin-* (bug?)
  sed -i.plg 's|lib/plugin|lib/kshark/plugin|g' src/KsUtils.cpp

===

4. CMake appears to build a debug build when you do not pass in CMAKE_BUILD_TYPE=Release, which ends up with more hard-coded build directory data in the compiled objects. I just updated it to build off of the other work in the above commit:

  # pass down Release as build type
  sed -i.cip 's|\(-D_INSTALL_PREFIX=\)|-DCMAKE_BUILD_TYPE=Release \1|g' \
    Makefile

This is just to get the job done as a packager, we probably want that to somehow key off the _DEBUG stuff (etc.) so that it's dynamic for developers.

===

5. Following the above, the CMakeLists.txt has a hard-coded -g in the flags which causes debug builds, which again hard-codes build directory paths in the resulting objects.

  # 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

Ideally, using the documented _DEBUG=1 should control this in some other cmake friendly way, this is just a quick packager hack.

===

6. Following 1 and 2 above, there are a few problematic paths when the cmake system tries to replace KS_DIR in files. I see this as a continuation of commit b9db93c5 and just replacing more KS_DIR with INSTALL_PREFIX:

  # KS_DIR is the random local build directory, relocate it to /usr
  sed -i.ks 's|@KS_DIR@|@_INSTALL_PREFIX@/share/kernelshark|g' \
    build/ks.desktop.cmake
  sed -i.ks 's|@KS_DIR@|@_INSTALL_PREFIX@|g' build/deff.h.cmake

The above actually fixes 2 problems - replacing KS_DIR with INSTALL_PREFIX, but also adding/fixing the location of where the kernelshark icon should be in the standard path (PREFIX/share/kernelshark/icons/) -- so:

6.5 - to match the above, the icon is not getting installed. As a packager, I just tossed this at the end of my build:

  # referenced in the .desktop file
  install -Dm0644 "${srcdir}/trace-cmd/kernel-shark/icons/ksharkicon.png" \
    "${pkgdir}/usr/share/kernelshark/icons/ksharkicon.png"

Again, quick hack - belongs in the Makefile somehow. :)

===

7. The value of TRACECMD_BIN_DIR gets set to the build directory, not what we want. My quick hack fix version of this:

  sed -i.tb 's|@TRACECMD_BIN_DIR@|@_INSTALL_PREFIX@/bin|g' build/deff.h.cmake

This again builds off commit b9db93c5 to leverage _INSTALL_PREFIX.

===

8. Last one, after all of the above work the use of KS_CONF_DIR is not really needed (item 1 makes it dynamic), this just gets the build directory out of the way and sets it to /tmp:

  # this ends up hard-coded to the build directory, with the above
  # patches it's no longer useful so set it to a safe directory
  sed -i.ksc 's|KS_CONF_DIR "\${KS_DIR}|KS_CONF_DIR "/tmp|g' CMakeLists.txt

This is a throwaway -- I believe if the above items are fixed, the need to even define, need or use KS_CONF_DIR just goes away completely. Temporary hack just for now.

===


With the above, it's a pretty clean install and I can successfully load, restore and save sessions (ends up in $HOME/.config/kernelshark/) as well as use my home directory when saving or loading trace files previously recorded. The packaging system now reports (or, doesn't report) that all libraries are clean from embedded build directory chaff - nice and clean.

===============
$ pacman -Ql trace-cmd-git

trace-cmd-git /etc/
trace-cmd-git /etc/bash_completion.d/
trace-cmd-git /etc/bash_completion.d/trace-cmd.bash
trace-cmd-git /usr/
trace-cmd-git /usr/bin/
trace-cmd-git /usr/bin/kernelshark
trace-cmd-git /usr/bin/kshark-record
trace-cmd-git /usr/bin/kshark-su-record
trace-cmd-git /usr/bin/trace-cmd
trace-cmd-git /usr/lib/
trace-cmd-git /usr/lib/kshark/
trace-cmd-git /usr/lib/kshark/libkshark-gui.so.0.9.8
trace-cmd-git /usr/lib/kshark/libkshark-plot.so.0.9.8
trace-cmd-git /usr/lib/kshark/libkshark.so.0.9.8
trace-cmd-git /usr/lib/kshark/plugin-missed_events.so
trace-cmd-git /usr/lib/kshark/plugin-sched_events.so
trace-cmd-git /usr/lib/trace-cmd/
trace-cmd-git /usr/lib/trace-cmd/plugins/
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_blk.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_cfg80211.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_function.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_futex.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_hrtimer.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_jbd2.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_kmem.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_kvm.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_mac80211.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_python.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_sched_switch.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_scsi.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_tlb.so
trace-cmd-git /usr/lib/trace-cmd/plugins/plugin_xen.so
trace-cmd-git /usr/lib/trace-cmd/python/
trace-cmd-git /usr/lib/trace-cmd/python/ctracecmd.so
trace-cmd-git /usr/lib/trace-cmd/python/event-viewer.py
trace-cmd-git /usr/lib/trace-cmd/python/tracecmd.py
trace-cmd-git /usr/lib/trace-cmd/python/tracecmdgui.py
trace-cmd-git /usr/share/
trace-cmd-git /usr/share/applications/
trace-cmd-git /usr/share/applications/kernelshark.desktop
trace-cmd-git /usr/share/kernelshark/
trace-cmd-git /usr/share/kernelshark/html/
trace-cmd-git /usr/share/kernelshark/html/images/
trace-cmd-git /usr/share/kernelshark/html/images/kshark-cursor-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-advance-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-del-adv.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-event-adv-list.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-events-sched.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-events.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-list-adv-irq.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-sync-dialog.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-sync-graph-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter-task-menu.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-filter.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-graph-info-line.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-graph-plot-area.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-graph-plot-title.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-list-adjust.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-list-enable-filter-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-list-graph-follow-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-list-graph-follow-2.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-list-info-area.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-open.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-cpu-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-cpu-2.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-cpu-result.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-menu.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-task-measure-preempt.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-task-measure.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-task-result.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-task-select.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-plot-task-zoom-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-select-a-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-select-b-1.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-unsync-events.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-zoom-in-2.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-zoom-in-3.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-zoom-in-select.png
trace-cmd-git /usr/share/kernelshark/html/images/kshark-zoom-out-select.png
trace-cmd-git /usr/share/kernelshark/html/index.html
trace-cmd-git /usr/share/kernelshark/icons/
trace-cmd-git /usr/share/kernelshark/icons/ksharkicon.png
trace-cmd-git /usr/share/man/
trace-cmd-git /usr/share/man/man1/
trace-cmd-git /usr/share/man/man1/kernelshark.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-check-events.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-extract.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-hist.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-list.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-listen.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-mem.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-options.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-profile.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-record.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-report.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-reset.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-restore.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-show.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-snapshot.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-split.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-stack.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-start.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-stat.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-stop.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd-stream.1.gz
trace-cmd-git /usr/share/man/man1/trace-cmd.1.gz
trace-cmd-git /usr/share/man/man5/
trace-cmd-git /usr/share/man/man5/trace-cmd.dat.5.gz
trace-cmd-git /usr/share/polkit-1/
trace-cmd-git /usr/share/polkit-1/actions/
trace-cmd-git /usr/share/polkit-1/actions/org.freedesktop.kshark-record.policy
===============


Direct packaging source links:
 https://aur.archlinux.org/cgit/aur.git/tree/PKGBUILD?h=trace-cmd-git
 https://aur.archlinux.org/cgit/aur.git/tree/001_ksdir_ksmainwindow.patch?h=trace-cmd-git
Comment 1 Steven Rostedt 2019-04-25 02:20:20 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
Comment 2 Yordan Karadzhov 2019-04-25 07:19:41 UTC
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
Comment 3 Troy Engel 2019-04-26 00:37:00 UTC
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.
Comment 4 Steven Rostedt 2019-04-26 01:03:30 UTC
(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
Comment 5 Yordan Karadzhov 2019-05-02 12:34:16 UTC
(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.
Comment 6 Troy Engel 2019-06-07 14:48:34 UTC
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.
Comment 7 Steven Rostedt 2019-06-10 19:24:07 UTC
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
Comment 8 Yordan Karadzhov 2019-06-11 10:35:36 UTC
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
Comment 9 Troy Engel 2019-06-11 11:49:50 UTC
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!
Comment 10 Troy Engel 2019-06-12 23:41:46 UTC
(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)...
Comment 11 Yordan Karadzhov 2019-06-14 14:21:30 UTC
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
Comment 12 Troy Engel 2019-06-14 23:37:11 UTC
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.
Comment 13 Troy Engel 2019-06-15 14:58:32 UTC
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.