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

Incorrect cache behavior for nixpkgs_package #17

Closed
guibou opened this issue Jun 12, 2018 · 10 comments
Closed

Incorrect cache behavior for nixpkgs_package #17

guibou opened this issue Jun 12, 2018 · 10 comments

Comments

@guibou
Copy link
Contributor

guibou commented Jun 12, 2018

nixpkgs_package caching is based on the parameters of the rule, such as nix_file or nix_file_content. They can reference other files which are not taken into account by the cache.

This mean that any change in these files does not invalidate the cache.

Proposed new design (I'll evaluate feasibility soon, bazel needs a way to force a rule to be run and control over the caching mecanism), the rule should always been build, but the caching / invalidation should be based on the result of the underlying nix-build command.

@guibou guibou changed the title Inexact of nixpkgs_package caching. Incorrect cache behavior for nixpkgs_package Jun 12, 2018
@Fuuzetsu
Copy link
Collaborator

I see the problem but I don't know the solution. I suspect that bazel should be running the command in some sort of sandbox where external files are not available unless they are part of the rule input at which point they are cached as usual. This is the only sane way I can think of.

rule should always been build, but the caching / invalidation should be based on the result of the underlying nix-build command

bazelbuild/bazel#3041

@Fuuzetsu
Copy link
Collaborator

side-note: I don't have ability to label in this repo, can I get that back? Thanks.

@mboes
Copy link
Member

mboes commented Jun 13, 2018

@Fuuzetsu should be good now. Had forgotten about this repo sorry.

@Fuuzetsu Fuuzetsu added the bug label Jun 13, 2018
@guibou
Copy link
Contributor Author

guibou commented Jun 13, 2018

bazelbuild/bazel#3041

There is no much activity in this area since a few, so I guess that we can conclude that bazel won't provide a solution in a short delay.

A pragmatic solution can be to add a data attribute to nipkgs_rules which list all the transitive file dependencies. It will be error prone, but at least we'll get a correct cache behavior if this attribute is correctly setup.

@Fuuzetsu
Copy link
Collaborator

I wouldn't be against such a well-documented workaround.

@facundominguez
Copy link
Member

Is it possible to have a rule modify one of its inputs? Maybe if it is not sandboxed? If so, it could be used to trigger any other rule that depends on it every time that bazel is executed.

guibou added a commit that referenced this issue Jun 13, 2018
This is a solution for #17, the new `data` attribute references all the files
needed to execute the nix expression, hence invalidating cache if they are
modified.
@mboes
Copy link
Member

mboes commented Jun 14, 2018

#18 only partially solves this. There is another issue orthogonal to what #18 resolves but related to this ticket. If <nixpkgs> changes, say following a nix-channel --update, then the cache won't be invalidated. Currently, we implicitly use <nixpkgs> if no repository attribute was provided. If we remove that feature, and force the user to always name a repository by git tag or commit, then we avoid that problem.

@mboes
Copy link
Member

mboes commented Jun 14, 2018

Note that nix-build, even when it's a noop, still costs a fair amount of time. So we should avoid running it unconditionally.

@guibou
Copy link
Contributor Author

guibou commented Jun 14, 2018

@mboes I'll do a second PR to remove the implicit <nixpkgs>.

Note that nix-build, even when it's a noop, still costs a fair amount of time. So we should avoid running it unconditionally.

For now this is not an issue because, as you said, #18 does not implement the proposed design. I think it is not possible until bazel proposes a solution for bazelbuild/bazel#3041.

mboes pushed a commit that referenced this issue Jun 17, 2018
This is a solution for #17, the new attribute references all the files
needed to execute the nix expression, so that the cache can properly
be invalidated if they change.
@mboes
Copy link
Member

mboes commented Aug 8, 2018

We can close this, since #18 and the extra warning are sufficient. See #24 for further steps we could take.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants