Hi, Christopher Baines skribis: > Ludovic Courtès writes: > >> Hi, >> >> Christopher Baines skribis: >> >>> Rather than having valid-narinfo? evaluate to #t if >>> %allow-unauthenticated-substitutes? is set to #t, just use (const #t) for >>> valid-narinfo? when %allow-unauthenticated-substitutes? is set to #t. This >>> will allow moving valid-narinfo? in to a (guix substitutes) module. >>> >>> * guix/scripts/substitute.scm (process-query, process-substitution): Change >>> the authorized? argument to lookup-narinfo and lookup-narinfos/diverse based >>> on %allow-unauthenticated-substitutes?. >>> (valid-narinfo?): Remove use of %allow-unauthenticated-substitutes?. >> >> Bummer that there are two call sites. >> >> What about doing away with ‘%allow-unauthenticated-substitutes?’ and >> instead changing its only user, ‘tests/substitute.scm’, like so: My bad, I missed that ‘test-env’ does: GUIX_ALLOW_UNAUTHENTICATED_SUBSTITUTES=yes So what I proposed won’t work. All in all, let’s just take the patch you proposed. Sorry for the confusion! > I don't know what's up with these tests in particular, adding peek in > places makes tests fail... not using Guile debugging helpers and > outputting to (current-error-port) seems to not change the result > though. Yeah that’s because (current-output-port) is used to communicate with the daemon, so if you inadvertently write things there, it breaks. > I didn't really understand this code, but looking at it more, I'm > thinking now that what it actually does is affects all the tests, and > for some tests in the (tests substitute) module, the > %allow-unauthenticated-substitutes? behaviour is turned off. Yeah, I got the logic wrong. > Commenting out the relevant code in the script seems to support this, > the substitute tests still pass, but tests in the store, derivation and > guix-daemon modules fail. The substitute tests are actually fine, and > break if you disable substitute authentication. The mock approach is > probably feasible, but it would need to be done in those > modules/tests. I haven't looked at the details, but I'd be a little > concerned that it might require mocking in each of the individual 15 > failing tests, maybe that's good for being explicit though? > > Back to the use of %allow-unauthenticated-substitutes? in the code, > there are two call sites, for the two separate code paths, but it would > be pretty easy to move to one call site. Both process-query and > process-substitution take an acl, but they could instead take some > (valid? obj) procedure. That would either call (valid-narinfo? obj acl) > or just evaluate to #t in the allow unauthorized case. This effectively > moves the logic and call site to the command. Yeah but the patch you proposed is fine. Thanks and apologies for the back-and-forth! Ludo’.