lix-project/nix-eval-jobs
2
1
Fork 2
Code Issues 2 Pull requests 1 Packages Projects Releases Wiki Activity

update and fix build with latest Lix #14

Open
qyriad wants to merge 2 commits from fixes/build-settings-opeq into main
pull from: fixes/build-settings-opeq
merge into: lix-project:main
Conversation 0 Commits 2 Files changed 2 +5 -5
1 changed files with 5 additions and 5 deletions
Show stats Download patch file Download diff file Expand all files Collapse all files
Showing only changes of commit 43aaa943bf - Show all commits

10
src/nix-eval-jobs.cc
View file

@ -350,18 +350,18 @@ int main(int argc, char **argv) {
/* FIXME: The build hook in conjunction with import-from-derivation is
* causing "unexpected EOF" during eval */
settings.builders = "";
settings.builders.setDefault("");
alois31 commented 2024-10-26 09:58:08 +00:00
Review
Copy link

Not sure if this makes sense, if it breaks stuff it should be overridden, and if it doesn't why do it at all?

Not sure if this makes sense, if it breaks stuff it should be overridden, and if it doesn't why do it at all?
/* Prevent access to paths outside of the Nix search path and
to the environment. */
evalSettings.restrictEval = false;
evalSettings.restrictEval.setDefault(false);
alois31 commented 2024-10-26 10:01:30 +00:00
Review
Copy link

This does not make sense at all, as the default is false already. You either want to override it or delete this entirely. Or judging by the comment, false is a typo to begin with and this should set the default to true (but still allowing the user to override it)?

This does not make sense at all, as the default is `false` already. You either want to override it or delete this entirely. Or judging by the comment, `false` is a typo to begin with and this should set the default to `true` (but still allowing the user to override it)?
qyriad commented 2024-10-27 20:47:25 +00:00
Review
Copy link

Oh that comment makes this very confusing then. The CLI doesn't seem to have any way to specify this in the first place? Should we just leave it as false since that's what it's been, and add a FIXME?

Oh that comment makes this very confusing then. The CLI doesn't seem to have any way to specify this in the first place? Should we just leave it as `false` since that's what it's been, and add a FIXME?
alois31 commented 2024-10-29 17:34:20 +00:00
Review
Copy link

Looking at the blame of this code, it seems that the comment was added along with explicitly setting this to true, and when it was later set to false some time later (presumably because breakage in some situations was noticed), the comment was left. I'm not really qualified to comment on this because I'm not familiar with this codebase, but my gut feeling is that this code should be deleted entirely and the user can pass --option restrict-eval true if they really want to enable restricted eval mode.

Looking at the blame of this code, it seems that the comment was added along with explicitly setting this to `true`, and when it was later set to `false` some time later (presumably because breakage in some situations was noticed), the comment was left. I'm not really qualified to comment on this because I'm not familiar with this codebase, but my gut feeling is that this code should be deleted entirely and the user can pass `--option restrict-eval true` if they really want to enable restricted eval mode.
/* When building a flake, use pure evaluation (no access to
'getEnv', 'currentSystem' etc. */
if (myArgs.impure) {
evalSettings.pureEval = false;
evalSettings.pureEval.setDefault(false);
alois31 commented 2024-10-26 10:03:06 +00:00
Review
Copy link

This wants to be override (and I guess things would be easier to understand here without the weird else if).

This wants to be override (and I guess things would be easier to understand here without the weird `else if`).
} else if (myArgs.flake) {
evalSettings.pureEval = true;
evalSettings.pureEval.setDefault(true);
}
if (myArgs.releaseExpr == "")
@ -374,7 +374,7 @@ int main(int argc, char **argv) {
}
if (myArgs.showTrace) {
loggerSettings.showTrace.assign(true);
loggerSettings.showTrace.override(true);
}
Sync<State> state_;