-
Notifications
You must be signed in to change notification settings - Fork 84
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
Improve symlink and refactor #27
Improve symlink and refactor #27
Conversation
`nix-build` by default creates a symlink to the store path in the current directory.
We print all parts of `exec_result` for nicer debugging.
Adds `-mindepth 1` to find, because otherwise the parent directory would always be symlinked as well. `-print0` is used to be stable in the face of filenames that could possibly contain the `\n` character.
nixpkgs/nixpkgs.bzl
Outdated
|
||
res = ctx.execute(["find", output_path, "-maxdepth", "1"]) | ||
res = ctx.execute([find_path, output_path, "-maxdepth", "1"]) |
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 actually fixes a bug in some situations, find_path
should have been used here all along.
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 looks great. One thing I'm worried about: if we pass --no-out-link
, won't that wrongly make the nixpkgs_package
eligible for GC? I believe we should create an out link to guard against that, though an out-link from within the bazel cache. So that when the user calls bazel clean
, then the package can be reclaimed by garbage collection again.
That’s a great idea. I think we need to create the out link in the build folder, which might clash with derivation files in the worst case. Nix already has the problem and introduces a canonical path called |
It will create a garbage collection root as long as the symlink created by `--out-link` exists.
Added support in a new commit. An example:
|
This refactors a few things and removes a two bugs with symlinking.
I split it into a four consecutive commits with one standalone change each, that should be helpful for reviewing.