tweag
nixpkgs_package does not invalidate when a file not listed in nix_file_deps changes
Describe the bug
nix_file_deps tracks the list of file which must trigger a cache invalidation of nixpkgs_package when they change.
However, if a file is not listed in this list, bazel will still build the nixpkgs_package, but later changes to this
To Reproduce
git checkout 65b47e980fb9bfb05c1dff9c2229a77b7d891541
cd repro
nix-shell -A repro
# observe the value
# change the value in inc.nix
nix-shell -A repro
# observe that the value did not change
nix-shell -A clean
nix-shell -A repro
# observe that the value did change after a clean
Expected behavior
I'd like the change to be taken into account without the need to bazel clean --expunge. Note that adding the necessary files (i.e. default.nix and inc.nix) to nix_file_deps does the job of invalidating the build. However #74 was supposed to trigger an error if a mandatory file was not in nix_file_deps.
Environment
- OS name + version: linux nixos something
- Version of the code: see the associated changeset: 65b47e980fb9bfb05c1dff9c2229a77b7d891541
This issue is avoided when the nixpkgs repository is declared using nixpkgs_local_repository instead of referring to the file //:nixpkgs.nix directly. nixpkgs_local_repository correctly fails when inc.nix is not declared. See https://github.com/tweag/rules_nixpkgs/commit/93e706d7c5120adf336dc7e2aec3e73b74a53fb2.
My reading is that users should always use nixpkgs_git_repository or nixpkgs_local_repository to declare a nixpkgs repository rather than passing the file to repositories directly. The example in the README documents this correctly. However, the documentation on the repositories attribute on nixpkgs_package is not clear about this. I'm not sure how to properly check for this and throw an error when no nixpkgs_*_repository is used.
We have a test for this:
https://github.com/tweag/rules_nixpkgs/blob/8c098688bc52c39829f4629d0f27edd51571a771/WORKSPACE#L59-L65
And indeed, if the nix_file_deps attribute is commented out, the test still succeeds, even though it should not. That's a more minimal repro.
@aherrmann It indeed works with nixpkgs_localy_repository, I'm now hitting another problem, but unrelated: https://github.com/tweag/rules_nixpkgs/issues/136
I still think that the issue must be fixed for explict file in nixpkgs_package.
@mboes thank you for the more minimal repro. I spent a lot of time trynig to do a repro and did not realize the different between my repro and the already existing test was the explicit versus @ nixpkgs repo.
As is documented in the README, nix_file_deps refers to the dependencies of nix_file_content or nix_file, not repositories. Both the test above and @guibou's original repro misuse the nix_file_deps attribute.
We could use the same file-copying trick for repositories as we do for the nix_file deps. It would be redundant in the case where the repository file comes from a nixpkgs_*_repository. It would also be counter intuitive: it would mean that nix_file_deps has to be the transitive closure of not just Nix file dependencies, but also repository dependencies.
It's tempting to close as wontfix, since the documentation is explicit. To avoid future user confusion, we could deprecate direct file repositories. And force the use of nixpkgs_*_repository. Do repository rules now support providers?
Do repository rules now support providers?
Sadly they don't. Furthermore, one cannot directly depend on a repository rule. @nixpkgs is just a shorthand for @nixpkgs//:nixpkgs.
As is documented in the README, nix_file_deps refers to the dependencies of nix_file_content or nix_file, not repositories. Both the test above and @guibou's original repro misuse the nix_file_deps attribute.
The documentation is of poor quality here (and I wrote it).
We could use the same file-copying trick for repositories as we do for the nix_file deps. It would be redundant in the case where the repository file comes from a nixpkgs_*_repository. It would also be counter intuitive: it would mean that nix_file_deps has to be the transitive closure of not just Nix file dependencies, but also repository dependencies.
My point of view here is that bazel must invalidate if any of the file used in the process of nixpkgs_package change. There is nothing else which matters (from my point of view). The initial design of the nix_file_deps was to check that all used files during the nixpkgs_package process was indeed listed in nix_file_deps.
It's tempting to close as wontfix, since the documentation is explicit. To avoid future user confusion, we could deprecate direct file repositories. And force the use of nixpkgs_*_repository. Do repository rules now support providers?
I'll converted my codebase to the nixpkgs_local_repository when #136 will be fixed.
However, closing this as won't fix is from my point of view a bit dangerous if we do not also deprecate (in the code) the current API. The example is what is happening right now on my codebase. We were used repositories and nix_file_deps with direct files on nixpkgs_package rules. It was the only solution a while ago, and it was a working solution. And the changes introduced on how nix_file_deps is working results on non-reproducible build here, without us even knowing.
My point of view here is that bazel must invalidate if any of the file used in the process of nixpkgs_package change. There is nothing else which matters (from my point of view).
Not breaking abstractions matters too. It looks like it's not possible to build meaningful abstractions in repository rules, because we can't have a repository rule depend on another repository rule (only files). Given that, maybe replicating the cp trick to repository files is the least bad option. Thoughts @aherrmann @guibou?
It looks like it's not possible to build meaningful abstractions in repository rules, because we can't have a repository rule depend on another repository rule (only files). Given that, maybe replicating the
cptrick to repository files is the least bad option.
It does look like that. However, I'm not sure if that's a viable option in cases where a repository is a full nixpkgs checkout, e.g. a nixpkgs_git_repository on a nixpkgs revision. It sounds like this would copy the entire nixpkgs repo into every instance of nixpkgs_package, which seems like something we don't want to do.
I don't know nixpkgs_git_repository enough. The original implementation of nix_file_deps was simply ignoring everything in /nix/store because this is not supposed to change, which was solving the problem (and the nixpkgs tree was in the nix store because it was fetched using nix primitives).