-
Notifications
You must be signed in to change notification settings - Fork 269
[cache] Use provided nix bin cache from api #1932
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
Conversation
## Summary TSIA ## How was it tested? builds
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see a couple of comments
|
||
type Provider struct{} | ||
|
||
var singleton *Provider = &Provider{} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why do providers need to be singletons?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
They don't right now, but I'm hoping to add caching to avoid making multiple API calls. Will do this after testing end to end and understanding where the perf bottlenecks are.
func ensureTrustedUser(ctx context.Context) { | ||
// we need to ensure that the user can actually use the extra | ||
// substituter. If the user did a root install, then we need to add | ||
// the trusted user/substituter to the nix.conf file and restart the daemon. | ||
|
||
// This check is not perfect, so we still try to use the substituter even if | ||
// it fails | ||
|
||
// TODOs: | ||
// * Also check if cache is enabled in nix.conf | ||
// * Test on single user install | ||
// * Automate making user trusted if needed | ||
if !nix.IsUserTrusted(ctx) { | ||
ux.Fwarning( | ||
os.Stderr, | ||
"In order to use a custom nix cache you must be a trusted user. Please "+ | ||
"add your username to nix.conf (usually located at /etc/nix/nix.conf)"+ | ||
" and restart the nix daemon.", | ||
) | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you're just showing a warning when a user is not trusted then ensureTrustedUser
is not the best name for it.
Maybe checkTrustedUser
or alternatively we can return NixCacheConfig{}
if the user is not trusted.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can change the function name. The issue is that I'm not 100% confident we can catch all cases where you are actually allowed to use the substitutor, so returning an empty cache config would be wrong.
Especially since nix build
will still work in the bad case (it just shows a warning)
}, | ||
IsDev: build.IsDev, | ||
Stderr: d.stderr, | ||
Store: &jetstore.JetpackAPIStore{}, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is this change relevant to the PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is no longer used when we updated envsec dependency.
internal/nix/build.go
Outdated
cmd.Env = allowUnfreeEnv(os.Environ()) | ||
cmd.Env = append(cmd.Env, args.Env...) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this can be cmd.Env = append(allowUnfreeEnv(os.Environ()), args.Env...)
Summary
Note: There's a minor issue we need to fix with the access token. It's missing the audience which is needed by the API. Will fix in https://github.com/jetpack-io/opensource
How was it tested?
devbox add hello
on org that has nix cache. Observed it attempted to use the cache.