qcachegrind: init at 16.12.3 #23812

periklis · 2017-03-12T20:55:32Z

Motivation for this change

This is the pure qt-based version of kcachegrind for Non-KDE-users e.g. MacOS. kcachegrind is merged under #23766

Things done

Tested using sandboxing
(nix.useSandbox on NixOS,
or option build-use-sandbox in nix.conf
on non-NixOS)
Built on platform(s)
- NixOS
- macOS
- Linux
Tested compilation of all pkgs that depend on this change using nix-shell -p nox --run "nox-review wip"
Tested execution of all binary files (usually in ./result/bin/)
Fits CONTRIBUTING.md.

periklis · 2017-03-12T20:56:49Z

@orivej Here is the WIP of my PR. Currently qcachegrind segfaults during qt-plugin-load.

periklis · 2017-03-13T12:10:21Z

Ok now i can fully understand the segfaults. I missed the wrapQtProgram.

periklis · 2017-03-13T12:40:05Z

Fixed missing Qt wrapping
Added converters to $out/bin

rycee · 2017-03-13T22:04:03Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+{ stdenv, fetchurl, bash, cmake, qt57, graphviz,  qmakeHook, makeQtWrapper, makeWrapper }:
+
+stdenv.mkDerivation rec {
+  name = "kcachegrind-${version}";


Should the name not be qcachegrind?

the name is needed for the url and qcachegrind is a subpackage of kcachegrind for non-kde-environments.

Hmm, then I would suggest changing the name here to qcachegrind and change the source URL to http://download.kde.org/stable/applications/${version}/src/kcachegrind-${version}.tar.xz.

The package name should reflect what you actually get when installing the package and doing nix-env -i kcachegrind and ending up with qcachegrind may be confusing.

good point :) i've always used the attr through nix-env -iA :)

orivej · 2017-03-14T02:00:13Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+     cp -r converters/memprof2calltree $out/bin/memprof2calltree
+     cp -r converters/op2calltree $out/bin/op2calltree
+     cp -r converters/pprof2calltree $out/bin/pprof2calltree
+     chmod -R +x $out/bin/


To include converters, you should list their dependencies in build inputs so that fixup phase fixes their shebangs. (Users may disable them with e.g. qcachegrind.override { php = null; }.)

Dependencies to perl, php, python added.

orivej · 2017-03-14T02:02:07Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+  buildInputs = [ qt57.qtbase graphviz ];
+  nativeBuildInputs = [ qmakeHook makeQtWrapper makeWrapper ];
+
+  postInstall = stdenv.lib.optionalString (stdenv.isDarwin) ''


I think this should also be available on Linux, at least cgview. I'm going to help with that.

I could separate postInstall to build a .desktop for linux and an .app for macos. Let me known what you think.

orivej · 2017-03-14T02:06:10Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+  version = "16.12.2";
+
+  src = fetchurl {
+    url = "http://download.kde.org/stable/applications/${version}/src/${name}.tar.xz";


I wonder if qcachegrind should be a part of kdeApplications since it uses its sources.

I am not sure if it is possible at all to provide linux+macos outputs through the kdeWrapper. I think it is best for bost OSes to have the two packages separate, since the CMakelists.txt for linux depend on find_library(kde4) which will never work on MacOS. Thus, a simple separate qcachegrind package for non-linux users will be the alternative.

e.g.:

'' + stdenv.lib.optionalString (stdenv.isLinux) '' mkdir -p "$out/share/applications" cat > "$out/share/applications/cgview.desktop" << __EOF__ [Desktop Entry] Exec=$out/bin/cgview Name=Cachgrind View GenericName=Profiling Visualization Terminal=true Type=Application Categories=Qt;Development;Profiling; __EOF__ wrapQtProgram $out/bin/cgview cat > "$out/share/applications/qcachegrind.desktop" << __EOF__ [Desktop Entry] Exec=$out/bin/qcachegrind Name=QCachgrind GenericName=Profiling Visualization Terminal=false Type=Application Categories=Qt;Development;Profiling; __EOF__ wrapQtProgram $out/bin/qcachegrind ''

orivej · 2017-03-14T02:09:52Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+  '';
+
+  meta = with stdenv.lib; {
+    description = "QCachegrind is a Qt GUI to visualize profiling data.";


Violates convention.

periklis · 2017-03-14T07:04:32Z

Thx for review, i'll take care in the next couple of hours.

periklis · 2017-03-17T08:41:02Z

@orivej ping

orivej · 2017-03-19T05:58:00Z

I would suggest to apply these changes: orivej@qcachegrind

Now that qt5 is qt58 which is not yet available for darwin, you can't use pkgs.qmakeHook. You can use libsForQt57.callPackage to bring in qmakeHook compatible with Qt 5.7.
The version might as well be updated to 16.12.3.
Delete unused inputs: makeWrapper and graphviz. Yes, running qcachegrind may invoke graphviz, but simply mentioning graphviz in buildInputs will not help installed qcachegrind find it.
Do not lose permissions with cp -r , use cp -a or cp -p.
There seems to be no reason to install cgview as an app bundle — it is a terminal application that can not be launched from the menu.

periklis · 2017-03-20T09:21:24Z

@orivej: I fixed 2. - 5. and let libsForQt57, since qt58 is not darwin-compatible yet. I have to check and apply the patch 4904df8 to qt-5/5.8, which will have to be a separate PR.

orivej · 2017-03-20T09:35:32Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+{ stdenv, fetchurl, bash, cmake,
+  qt, graphviz,
+  qmakeHook, makeQtWrapper,
+  perl, python, php


According to the style guide (“Function formal arguments”), lines with arguments do not end with commas.

bash and graphviz are unused.

orivej · 2017-03-20T09:42:37Z

pkgs/development/tools/analysis/qcachegrind/default.nix

+     mkdir -p $out/Applications
+     cp -rp qcachegrind/qcachegrind.app $out/Applications
+     wrapQtProgram $out/Applications/qcachegrind.app/Contents/MacOS/qcachegrind
+     chmod -R +x $out/Applications/qcachegrind.app


This chmod is not needed.

You did not include installation of Linux desktop files.

Installation on linux should be let to kcachegrind?!

There is no cgview in kcachegrind package, and some people who do not use KDE might prefer qcachegrind.

Ok, as asked above. What is the preferred way, provide the binary only under $out/bin or a qcachegrind.desktop file?

I linked to what I would expect on Linux: an executable in $out/bin/qcachegrind and desktop metadata (for qcachegrind to appear in the launcher) in $out/share/applications.

periklis · 2017-03-22T13:42:50Z

@orivej any other impediments to merge this?

orivej · 2017-03-22T15:14:40Z

I approve the merge, but I do not have merge rights.

rycee · 2017-03-22T15:37:35Z

I'm doing a build of this now, if everything goes well I'll put it in master.

rycee · 2017-03-22T16:19:55Z

Rebased into master in a92dfe7. Many thanks @periklis for the submission and @orivej for the reviewing!

periklis force-pushed the topic_qcachegrind branch from d8a47a8 to 5262679 Compare March 13, 2017 12:39

periklis changed the title ~~[WIP] qcachegrind: init at 16.12.2~~ qcachegrind: init at 16.12.2 Mar 13, 2017

rycee reviewed Mar 13, 2017

View reviewed changes

rycee added 8.has: package (new) 6.topic: qt/kde labels Mar 13, 2017

orivej reviewed Mar 14, 2017

View reviewed changes

periklis force-pushed the topic_qcachegrind branch 5 times, most recently from d312332 to ddb11b8 Compare March 16, 2017 21:17

periklis force-pushed the topic_qcachegrind branch from ddb11b8 to efb209a Compare March 20, 2017 09:18

periklis force-pushed the topic_qcachegrind branch from efb209a to 79c43e9 Compare March 20, 2017 09:22

orivej suggested changes Mar 20, 2017

View reviewed changes

periklis changed the title ~~qcachegrind: init at 16.12.2~~ qcachegrind: init at 16.12.3 Mar 20, 2017

periklis force-pushed the topic_qcachegrind branch 2 times, most recently from d72ceea to 9a52fe6 Compare March 21, 2017 20:22

qcachegrind: init at 16.12.3

d61467a

periklis force-pushed the topic_qcachegrind branch from 9a52fe6 to d61467a Compare March 22, 2017 14:28

rycee closed this Mar 22, 2017

periklis deleted the topic_qcachegrind branch July 25, 2018 09:22

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

qcachegrind: init at 16.12.3 #23812

qcachegrind: init at 16.12.3 #23812

periklis commented Mar 12, 2017 •

edited

periklis commented Mar 12, 2017

periklis commented Mar 13, 2017

periklis commented Mar 13, 2017

rycee Mar 13, 2017

periklis Mar 14, 2017

rycee Mar 14, 2017

periklis Mar 14, 2017

periklis Mar 14, 2017

orivej Mar 14, 2017 •

edited

periklis Mar 14, 2017

orivej Mar 14, 2017

periklis Mar 14, 2017

orivej Mar 14, 2017

periklis Mar 14, 2017

periklis Mar 14, 2017

orivej Mar 14, 2017

periklis commented Mar 14, 2017

periklis commented Mar 17, 2017

orivej commented Mar 19, 2017 •

edited

periklis commented Mar 20, 2017

orivej Mar 20, 2017 •

edited

orivej Mar 20, 2017

periklis Mar 20, 2017

orivej Mar 20, 2017

periklis Mar 20, 2017

orivej Mar 20, 2017

periklis commented Mar 22, 2017

orivej commented Mar 22, 2017

rycee commented Mar 22, 2017 •

edited

rycee commented Mar 22, 2017

qcachegrind: init at 16.12.3 #23812

qcachegrind: init at 16.12.3 #23812

Conversation

periklis commented Mar 12, 2017 • edited

Motivation for this change

Things done

periklis commented Mar 12, 2017

periklis commented Mar 13, 2017

periklis commented Mar 13, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

orivej Mar 14, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Mar 14, 2017

periklis commented Mar 17, 2017

orivej commented Mar 19, 2017 • edited

periklis commented Mar 20, 2017

orivej Mar 20, 2017 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

periklis commented Mar 22, 2017

orivej commented Mar 22, 2017

rycee commented Mar 22, 2017 • edited

rycee commented Mar 22, 2017

periklis commented Mar 12, 2017 •

edited

orivej Mar 14, 2017 •

edited

orivej commented Mar 19, 2017 •

edited

orivej Mar 20, 2017 •

edited

rycee commented Mar 22, 2017 •

edited