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

Add page size system features #12446

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

RossComputerGuy
Copy link
Member

Motivation

Fixes #12426

Context

Prevents issues like NixOS/nixpkgs#348660 from occurring by adding system features which describe the page size. This has the added benefit of systems with multiple builders of utilizing only compatible page sizes.

Testing this PR involves building this PR and running Nix to build any derivation inside this branch of nixpkgs: https://github.com/RossComputerGuy/nixpkgs/tree/feat/page-size

It should not fail with this error:

error: a 'aarch64-linux' with features {pages-16k, pages-32k, pages-4k, pages-64k, pages-8k} is required to build '/nix/store/njf0b0lzfs9gdyylpdyb8gg6s9x0kq35-bootstrap-stage0-glibc-bootstrapFiles.drv', but I am a 'aarch64-linux' with features {benchmark, big-parallel, gccarch-armv8-a, kvm, nixos-test}

Add 👍 to pull requests you find important.

The Nix maintainer team uses a GitHub project board to schedule and track reviews.

@RossComputerGuy RossComputerGuy marked this pull request as draft February 11, 2025 07:17
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Feb 14, 2025
@RossComputerGuy RossComputerGuy marked this pull request as ready for review February 14, 2025 20:40
Copy link
Member

@roberth roberth left a comment

Choose a reason for hiding this comment

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

This might lead to a mandatoryRejectedSystemFeatures option and machine parameter, or that could be over-engineered.

@@ -370,6 +370,18 @@ Derivations can declare some infrequently used optional attributes.

ensures that the derivation can only be built on a machine with the `kvm` feature.

- [`rejectSystemFeatures`]{#adv-attr-rejectSystemFeatures}\

If a derivation has the `rejectSystemFeatures` attribute, then Nix will only build it on a machine that does not has the corresponding features set in its [`system-features` configuration](@docroot@/command-ref/conf-file.md#conf-system-features).
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
If a derivation has the `rejectSystemFeatures` attribute, then Nix will only build it on a machine that does not has the corresponding features set in its [`system-features` configuration](@docroot@/command-ref/conf-file.md#conf-system-features).
If a derivation has the `rejectSystemFeatures` attribute, then Nix will only build it on a machine that does not have the corresponding features set in its [`system-features` configuration](@docroot@/command-ref/conf-file.md#conf-system-features).

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for catching that, it's fixed now.

rejectSystemFeatures = [ "pages-16k" ];
```

ensures that the derivation can only be built on a machine which do not have the `pages-16k` feature.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
ensures that the derivation can only be built on a machine which do not have the `pages-16k` feature.
ensures that the derivation can only be built on a machine which does not have the `pages-16k` feature.

Copy link
Member Author

Choose a reason for hiding this comment

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

Done

@@ -532,9 +532,10 @@ void LocalDerivationGoal::startBuilder()

/* Right platform? */
if (!parsedDrv->canBuildLocally(worker.store))
throw Error("a '%s' with features {%s} is required to build '%s', but I am a '%s' with features {%s}",
throw Error("a '%s' with features {%s} & not {%s} is required to build '%s', but I am a '%s' with features {%s}",
Copy link
Member

Choose a reason for hiding this comment

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

Not pretty when empty, but that was already the case for required ones.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, I'm curious how we should handle things when it's empty.

@@ -70,6 +71,12 @@ echo "$output" | grepQuietInverse builder-build-remote-input-2.sh
echo "$output" | grepQuiet builder-build-remote-input-3.sh
unset output

# Ensure that input4 was built on store4 due to the required feature.
Copy link
Member

Choose a reason for hiding this comment

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

This isn't obvious from the code below. Could you add assertions that simply check what you're saying here?

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 getting a test here which would reject builds in the proper way but I'm not confident with the testing framework so I might need guidance.

@roberth
Copy link
Member

roberth commented Feb 19, 2025

It seems that #12292 introduced a conflict.
@Ericson2314 what is the status of #12410?

@RossComputerGuy RossComputerGuy force-pushed the feat/page-size branch 2 times, most recently from 75a730b to 0bcf588 Compare February 19, 2025 14:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation with-tests Issues related to testing. PRs with tests have some priority
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add a page size system feature
2 participants