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

graphene-hardened-malloc: migrate to by-name, build light variant #266540

Merged
merged 3 commits into from
Apr 26, 2024

Conversation

surfaceflinger
Copy link
Member

@surfaceflinger surfaceflinger commented Nov 9, 2023

Description of changes

Also added it to the malloc module :p

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/)
  • 23.11 Release Notes (or backporting 23.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.

@risicle
Copy link
Contributor

risicle commented Nov 9, 2023

Got to say I'm very much not a fan of the "by-name" layout.

@Kranzes
Copy link
Member

Kranzes commented Nov 10, 2023

Got to say I'm very much not a fan of the "by-name" layout.

Well there was an RFC for it for a reason..

@surfaceflinger
Copy link
Member Author

@risicle what do you want done then? The end goal is apparently having as many packages as possible in by-name...

@surfaceflinger surfaceflinger marked this pull request as draft November 11, 2023 16:49
@surfaceflinger surfaceflinger marked this pull request as ready for review November 11, 2023 16:51
@risicle
Copy link
Contributor

risicle commented Nov 11, 2023

Well there was an RFC for it for a reason..

An RFC doesn't (and cannot) produce unanimity.

But I can't stand in the way of the great scattering myself, so do as you like.

@Kranzes
Copy link
Member

Kranzes commented Nov 11, 2023

My point is that at the moment we don't have a policy in-place so it is up to yo,u but it seems like in the near future everything will be moved to by-name so your decision won't be that relevant longer term.

@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/1245

Comment on lines +26 to +32
buildPhase = ''
runHook preBuild

for VARIANT in default light; do make $makeFlags ''${enableParallelBuilding:+-j$NIX_BUILD_CORES} VARIANT=$VARIANT; done

runHook postBuild
'';
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 makeTargets instead

Copy link
Member Author

Choose a reason for hiding this comment

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

From my tinkering, looks like both makeTargets and makeFlags won't work with how hardened_malloc's Makefile handles VARIANT.

From what I experimented with, we have 2 variants left:

  • let mkMalloc = variant: stdenv.mkDerivation {}; in { graphene-hardened-malloc = mkMalloc "default"; graphene-hardened-malloc-light = mkMalloc "light"; }
    but this will create graphene-hardened-malloc.default and graphene-hardened-malloc.light + I don't like how these let blabla = stdenv.mkDerivation in look 🙄
  • we can introduce an overridable variant attribute like mimalloc package and then override it in the module.

nixos/modules/config/malloc.nix Show resolved Hide resolved
@surfaceflinger
Copy link
Member Author

surfaceflinger commented Apr 18, 2024

Rebased, rewrote descriptions. (more like pasted them from readme)

Also bumped the package from branch 12 to tag 2024040900 as upstream is using these tags to mark production-ready releases.

Regarding overriding buildPhase - I can't really think of anything better when it comes to how the upstream Makefile works, personally I would prefer to leave it as it is here unless someone has a better idea.

Copy link
Member

@pbsds pbsds left a comment

Choose a reason for hiding this comment

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

Result of nixpkgs-review pr 266540 run on x86_64-linux 1

2 packages blacklisted:
  • nixos-install-tools
  • tests.nixos-functions.nixos-test
2 packages built:
  • graphene-hardened-malloc
  • graphene-hardened-malloc.debug

@pbsds pbsds merged commit 01a730b into NixOS:master Apr 26, 2024
27 checks passed
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.

8 participants