Skip to content

Add eval-system option#4093

Merged
Ericson2314 merged 3 commits into
NixOS:masterfrom
matthewbauer:eval-system
Dec 15, 2023
Merged

Add eval-system option#4093
Ericson2314 merged 3 commits into
NixOS:masterfrom
matthewbauer:eval-system

Conversation

@matthewbauer

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

Copy link
Copy Markdown
Member

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

stale Bot commented Jun 2, 2021

Copy link
Copy Markdown

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

@stale stale Bot added the stale label Jun 2, 2021
@stale

stale Bot commented Jun 19, 2022

Copy link
Copy Markdown

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

roberth commented Jun 22, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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

Ericson2314 commented Jun 22, 2023

Copy link
Copy Markdown
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
Comment thread 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

@edolstra edolstra Jul 10, 2023 •
edited
Loading

Copy link
Copy Markdown
Member

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
Comment thread 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

@fricklerhandwerk fricklerhandwerk Jul 10, 2023

Copy link
Copy Markdown
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

Comment thread src/libstore/globals.hh Outdated
Comment thread src/libstore/globals.hh Outdated
@nixos-discourse

nixos-discourse commented Jul 15, 2023

Copy link
Copy Markdown

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
@Ericson2314 Ericson2314 requested a review from roberth as a code owner November 30, 2023 01:00
@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 Ericson2314 disabled auto-merge December 15, 2023 00:22
@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

Ericson2314 commented Dec 15, 2023

Copy link
Copy Markdown
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
tebowy pushed a commit to tebowy/nix that referenced this pull request Jul 11, 2024
Merge pull request NixOS#4093 from matthewbauer/eval-system
7614aa9 
Add eval-system option

(cherry picked from commit 071dbbe)
Change-Id: Ia81358c8cfb60241da07a4d0e84b9ee62a18a53f
@roberth roberth mentioned this pull request Aug 23, 2024
Open
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

@thufschmitt thufschmitt Awaiting requested review from thufschmitt

@roberth roberth Awaiting requested review from roberth

+1 more reviewer

@fricklerhandwerk fricklerhandwerk fricklerhandwerk left review comments

Reviewers whose approvals may not affect merge requirements

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.

6 participants

@matthewbauer @roberth @Ericson2314 @nixos-discourse @edolstra @fricklerhandwerk