Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

kernelshark: init at 0.9.8 & trace-cmd: 2.6 -> 2.8.3 #60367

Merged
merged 8 commits into from
Jul 28, 2019
Merged

kernelshark: init at 0.9.8 & trace-cmd: 2.6 -> 2.8.3 #60367

merged 8 commits into from
Jul 28, 2019
+127 −14

Conversation

basvandijk
Copy link
Member

@basvandijk basvandijk commented Apr 28, 2019 •
edited
Loading

Motivation for this change

https://www.phoronix.com/scan.php?page=news_item&px=KernelShark-1.0-Soon

Note that the kernelshark-1.0 has not been released yet. This packages the latest git revision.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nix-review --run "nix-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Sorry, something went wrong.

@basvandijk basvandijk changed the title wireshark: init at 0.9.8-a8faf36 & trace-cmd: 2.6 -> 2.8-a8faf36 kernelshark: init at 0.9.8-a8faf36 & trace-cmd: 2.6 -> 2.8-a8faf36 Apr 28, 2019
@teto
Copy link
Member

teto commented Apr 28, 2019

When I read about it on phoronix, I thought it would be cool to have it here but since there is no release yet, I wonder when to merge. Did they announce a release date yet ?

@ofborg ofborg bot added the 8.has: package (new) This PR adds a new package label Apr 28, 2019
@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from 56db710 to d3c2b37 Compare April 28, 2019 11:17
@ofborg ofborg bot requested a review from thoughtpolice April 28, 2019 11:17
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 labels Apr 28, 2019
jtojnar
jtojnar requested changes Apr 28, 2019
View reviewed changes
pkgs/os-specific/linux/trace-cmd/default.nix

MANPAGE_DOCBOOK_XSL="${docbook_xsl}/xml/xsl/docbook/manpages/docbook.xsl";

buildPhase = "make trace-cmd libs doc";
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please use

buildFlags = [ "trace-cmd" "libs" "doc" ];

Overriding phases like this breaks hooks and other features of the generic builder.

Copy link
Member Author

@basvandijk basvandijk Apr 28, 2019 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I tried to build with either buildFlags, makeFlags or buildTargets set to [ "trace-cmd" "libs" "doc" ]; but all resulted in a failing build.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It seems to be caused by generic builder adding SHELL variable:

nixpkgs/pkgs/stdenv/generic/setup.sh

Line 1027 in 9dd1bbc

SHELL=$SHELL

Which is extremely weird, since the makefiles do not seem to refer to SHELL. And even weider, the linking error is from not finding a symbol that is in the same file:

/nix/store/1kl6ms8x56iyhylb2r83lq7j3jbnix7w-binutils-2.31.1/bin/ld: /build/trace-cmd-a8faf36/tracecmd/trace-record.o: in function `reset_event_pid':
/build/trace-cmd-a8faf36/tracecmd/trace-record.c:3850: undefined reference to `add_event_pid'
collect2: error: ld returned 1 exit status

pkgs/os-specific/linux/trace-cmd/fix-Makefiles.patch

GUI_INSTALL = $(HTML_INSTALL) $(IMGS_INSTALL)

-install: $(MAN1_INSTALL) $(MAN5_INSTALL) $(GUI_INSTALL)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why split the targets?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For the trace-cmd package I only want to install the man pages and not the documentation for kernelshark ($(GUI_INSTALL)) so I added the install_man target.

@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from d3c2b37 to 988f080 Compare April 28, 2019 11:48
@basvandijk
Copy link
Member Author

@GrahamcOfBorg build trace-cmd kernelshark

@basvandijk basvandijk requested a review from jtojnar April 28, 2019 12:01
@basvandijk
Copy link
Member Author

I wonder when to merge. Did they announce a release date yet ?

@teto no, I don't think they announced a release date yet.

The reason I packaged it is that I want to try this out. Merging would allow other nixpkgs-unstable users to also try this out. However I do wonder myself what our policy is of packaging unreleased software in nixpkgs-unstable.

@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from 988f080 to 1827749 Compare May 10, 2019 16:14
@basvandijk basvandijk changed the title kernelshark: init at 0.9.8-a8faf36 & trace-cmd: 2.6 -> 2.8-a8faf36 kernelshark: init at 0.9.8-c8d9e1a & trace-cmd: 2.6 -> 2.8-c8d9e1a May 10, 2019
@basvandijk
Copy link
Member Author

@jtojnar could you do another review please?

@basvandijk
Copy link
Member Author

I reported the Makefile patches upstream. Hopefully they get merged so we don't have to maintain them.

@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from 1827749 to f0c5684 Compare June 3, 2019 22:00
@basvandijk
trace-cmd: 2.6 -> 2.8-c8d9e1a
a9b4881
@basvandijk
kernelshark: init at 0.9.8-c8d9e1a
fa5a346
@basvandijk
trace-cmd: 2.8-c8d9e1a -> 2.8-0c957d2
d1466e7
@basvandijk
trace-cmd: 2.8-0c957d2 -> 2.9-dev-1517dc3

Verified

This commit was created on GitHub.com and signed with GitHub’s verified signature. The key has expired.
GPG key ID: 4AEE18F83AFDEB23
Expired
Verified
Learn about vigilant mode
4542cca
@basvandijk
kernelshark: install documentation
585de35
@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from f0c5684 to 585de35 Compare July 25, 2019 20:13
@basvandijk
Copy link
Member Author

@GrahamcOfBorg build trace-cmd kernelshark

@basvandijk basvandijk changed the title kernelshark: init at 0.9.8-c8d9e1a & trace-cmd: 2.6 -> 2.8-c8d9e1a kernelshark: init at 0.9.8-1517dc3 & trace-cmd: 2.6 -> 2.9-dev-1517dc3 Jul 25, 2019
@basvandijk basvandijk force-pushed the trace-cmd-and-kernelshark branch from b8bfc5c to c9c8950 Compare July 27, 2019 18:34
@basvandijk basvandijk changed the title kernelshark: init at 0.9.8-1517dc3 & trace-cmd: 2.6 -> 2.9-dev-1517dc3 kernelshark: init at 0.9.8 & trace-cmd: 2.6 -> 2.8.3 Jul 27, 2019
@basvandijk
Copy link
Member Author

@GrahamcOfBorg build trace-cmd kernelshark

@basvandijk basvandijk merged commit 1ff227a into NixOS:master Jul 28, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@thoughtpolice thoughtpolice

@jtojnar jtojnar

Assignees
No one assigned
Labels
8.has: package (new) This PR adds a new package 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@basvandijk @teto @jtojnar