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.

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

gperftools: split output #334825

Open
wants to merge 1 commit into
base: staging
Choose a base branch
from
Open

Conversation

matko
Copy link
Contributor

@matko matko commented Aug 15, 2024

Description of changes

This PR splits the output of gperftools, to allow applications that link to tcmalloc (provided by gperftools) to not also depend on the profiling tools, which would pull in perl. Additionally, the minimal dynamic tcmalloc library (libtcmalloc_minimal.so on linux) is moved into its own dedicated output, which further allows dropping libunwind for applications that only need a replacement allocator.

Additionally, static libraries get moved to a special output 'static' which goes last on the propagated build output list, ensuring that dynamic linking dependents can drop the static libraries too, and naive linking using the cc wrappers and NIX_LDFLAGS will prioritize dynamic over static linking.

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@tobim
Copy link
Contributor

tobim commented Aug 16, 2024

Nothing against this, but wouldn't packaging https://github.com/google/tcmalloc/blob/master/docs/gperftools.md also be a good alternative?

@matko
Copy link
Contributor Author

matko commented Aug 16, 2024

Maybe that is possible too. I don't really know the extent of the differences between these two versions though, and I worry that replacing the version of tcmalloc with the slightly different google version may lead to surprises down the line (such as errors that only seem to happen on Nix but which can't be reproduced on dev machines running something else).

For what it's worth, getting tcmalloc_minimal on Debian would get you the version from gperftools. It looks like the same goes for Fedora. I think it's therefore reasonable to assume that most (non-google) packages depending on tcmalloc would be expecting to get it from gperftools.

@matko matko force-pushed the gperftools_split_output branch 2 times, most recently from 6d6744a to 5fed533 Compare August 16, 2024 13:11
@matko
Copy link
Contributor Author

matko commented Aug 27, 2024

Giving this PR a nudge since it has been here for about 2 weeks.

@vcunat could you have a look?

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1953

@phanirithvij
Copy link
Member

should this target staging?

@matko
Copy link
Contributor Author

matko commented Sep 14, 2024

Should it? I'm new to this so I'm not very familiar yet with the nixpkgs development process. I also had no idea that this would affect so many packages when I made the PR.
Please let me know if I need to change anything or reopen this PR to target another branch.

@matko matko marked this pull request as draft September 14, 2024 12:18
@matko matko changed the base branch from master to staging September 14, 2024 12:22
@matko matko marked this pull request as ready for review September 14, 2024 12:24
@matko matko force-pushed the gperftools_split_output branch from 5fed533 to 6353ebe Compare September 14, 2024 13:15
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 27, 2024
@matko matko force-pushed the gperftools_split_output branch from 6353ebe to f779f4e Compare September 30, 2024 11:13
@matko
Copy link
Contributor Author

matko commented Sep 30, 2024

Repushed to resolve conflict with tree-wide change.

@ofborg ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Sep 30, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Nov 9, 2024
@matko
Copy link
Contributor Author

matko commented Dec 3, 2024

Another two months passed and a new version of NixOS was released in the meantime. is there any chance of this getting merged or should I just close?

@tobim
Copy link
Contributor

tobim commented Dec 3, 2024

I'd like to see this merged, but I can't do that myself.

@matko
Copy link
Contributor Author

matko commented Dec 3, 2024

This will need another fixup since gperftools got moved into the by-name hierarchy. I'm not sure if it makes sense to keep doing this here or to open a new PR though.
And the whole thing is pointless if nobody is going to merge it anyway.

@phanirithvij
Copy link
Member

phanirithvij commented Dec 4, 2024

And the whole thing is pointless if nobody is going to merge it anyway.

You can post it on discourse "prs ready for review" and matrix #review-requests channels. And reach out to @vcunat on matrix or discourse for a review request.
It is not pointless, please don't think that way, everyone with a commit access is a volunteer and I'm sure you already know that.

I'm not sure if it makes sense to keep doing this here or to open a new PR though.

You can do it in this pr.

Just a headsup.

There is a treewide nixfmt pr which will conflict with this soon https://discourse.nixos.org/t/nix-formatting-team-treewide-nixpkgs-formatting/56666 as it is touching by-name/gperftools/package.nix as well. It will get merged on Dec 11 (mentioned in the discourse post)
So you might have to fix conflicts again or you can adapt it right now.

And I am not a commiter, just came across your pr.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants