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

Jump to bottom

Add eval-system option #4093

Merged
merged 3 commits into from Dec 15, 2023
Merged

Add eval-system option #4093

merged 3 commits into from Dec 15, 2023

Conversation

matthewbauer
Copy link
Member

@matthewbauer matthewbauer commented Sep 29, 2020 •
edited by Ericson2314

Add a new eval-system option.
Unlike system, it just overrides the value of builtins.currentSystem.
This is more useful than overriding system, because you can build these derivations on remote builders which can work on the given system.
In contrast, system also effects scheduling which will cause Nix to build those derivations locally even if that doesn't make sense.

eval-system only takes effect if it is non-empty. If empty (the default) system is used as before, so there is no breakage.

@matthewbauer matthewbauer force-pushed the eval-system branch 2 times, most recently from bc096f7 to 61b5809 Compare September 29, 2020 19:53
@stale
Copy link

stale bot commented Jun 2, 2021

I marked this as stale due to inactivity. → More info

@stale stale bot added the stale label Jun 2, 2021
@stale
Copy link

stale bot commented Jun 19, 2022

I closed this issue due to inactivity. → More info

@stale stale bot closed this Jun 19, 2022
@roberth roberth mentioned this pull request Jun 16, 2023
Merged
8 tasks
@roberth roberth reopened this Jun 22, 2023
@stale stale bot removed stale labels Jun 22, 2023
@roberth
Copy link
Member

roberth commented Jun 22, 2023 •
edited

Still relevant. Setting system for the purpose of creating .drv files is rather different from setting system for the purpose of specifying what can be built locally.
There's some problems you'd have to work around if you set system. I don't recall exactly the conditions or error messages, but basically you're confusing the (store layer) scheduling about what it can build locally.

@matthewbauer not sure if you have time to rebase, so feel free to wait until the Nix team approves the goal/implementation strategy first. It does seem like a simple PR fwiw.

@roberth roberth added language The Nix expression language; parser, interpreter, primops, evaluation, etc scheduling settings Settings, global flags, nix.conf labels Jun 22, 2023
@Ericson2314
Copy link
Member

Oh this is good I think. We can put this in EvalSettings, default it based on settings.system, and only set builtins.currentSystem based on this.

This removes the incentive to start talking about builtins.currentSystem in the libstore settings, which would be a layer violation. It also makes it easier to get rid of settings layer as we should not have global variables that only make sense for certain stores not others.

edolstra
edolstra reviewed Jul 10, 2023
View reviewed changes
src/libstore/globals.hh Outdated
report when evaluating Nix expressions. This can be accessed
via builtins.currentSystem. Unlike `system`, this setting
does not change what kind of derivations can be built
locally. This is useful for evaluating Nix on your system
Copy link
Member

@edolstra edolstra Jul 10, 2023 •
edited

Choose a reason for hiding this comment

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

"evaluating Nix": should be evaluating Nix code or something like that.

Actually the whole sentence should be rephrased to something like "This is useful for producing derivations to be built on another type of system."

fricklerhandwerk
fricklerhandwerk reviewed Jul 10, 2023
View reviewed changes
src/libstore/globals.hh Outdated
R"(
This option specifies the canonical Nix system name to
report when evaluating Nix expressions. This can be accessed
via builtins.currentSystem. Unlike `system`, this setting
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
via builtins.currentSystem. Unlike `system`, this setting
via [`builtins.currentSystem`](@docroot@/language/builtins-constants.md#builtin-constants-currentSystem). Unlike [`system`](#conf-system), this setting

src/libstore/globals.hh Outdated Show resolved Hide resolved
src/libstore/globals.hh Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

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

https://discourse.nixos.org/t/2023-07-10-nix-team-meeting-minutes-70/30471/1

@roberth roberth mentioned this pull request Jul 18, 2023
Merged
8 tasks
@github-actions github-actions bot added the with-tests Issues related to testing. PRs with tests have some priority label Nov 30, 2023
@matthewbauer @Ericson2314 @fricklerhandwerk
Add eval-system option
bcbdb09 
`eval-system` option overrides just the value of `builtins.currentSystem`.
This is more useful than overriding `system` since you can build these
derivations on remote builders which can work on the given system.

Co-authored-by: John Ericson <John.Ericson@Obsidian.Systems>
Co-authored-by: Valentin Gagarin <valentin.gagarin@tweag.io>
@Ericson2314
Functional Test for builtins.storeDir and builtins.currentSystem
70f50cb 
These were under-tested. This tests the status quo and especially
previous commit of this PR better.
@Ericson2314
Add release not for eval-system
228e995
@Ericson2314
Copy link
Member

I forgot tests and release note. Now they added, and this is ready to merge.

@Ericson2314 Ericson2314 merged commit 071dbbe into NixOS:master Dec 15, 2023
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edolstra edolstra edolstra left review comments

@fricklerhandwerk fricklerhandwerk fricklerhandwerk left review comments

@thufschmitt thufschmitt Awaiting requested review from thufschmitt thufschmitt is a code owner

@roberth roberth Awaiting requested review from roberth roberth is a code owner

Assignees

@Ericson2314 Ericson2314

Labels
language The Nix expression language; parser, interpreter, primops, evaluation, etc scheduling settings Settings, global flags, nix.conf with-tests Issues related to testing. PRs with tests have some priority
Projects
Nix team
Archived in project
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
6 participants
@matthewbauer @roberth @Ericson2314 @nixos-discourse @edolstra @fricklerhandwerk