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

Conversation

basvandijk
Copy link
Member

@basvandijk basvandijk commented Apr 28, 2019 •
edited

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.

@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 requested a review from thoughtpolice April 28, 2019 11:17
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
Contributor

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

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
Contributor

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
Contributor

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.

pkgs/os-specific/linux/trace-cmd/fix-Makefiles.patch Outdated Show resolved Hide resolved
pkgs/os-specific/linux/trace-cmd/kernelshark.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/trace-cmd/default.nix Outdated Show resolved Hide resolved
pkgs/os-specific/linux/trace-cmd/default.nix Outdated Show resolved Hide resolved
@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 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
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 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 Awaiting requested review from thoughtpolice

@jtojnar jtojnar Awaiting requested review from jtojnar

Assignees
No one assigned
Labels
8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
3 participants
@basvandijk @teto @jtojnar