-
Notifications
You must be signed in to change notification settings - Fork 83
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
Introduce a data
attribute to nixpkgs_package
#18
Conversation
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.
The extra sources files are not really data. They're extra source files. While #17 seems to claim a more general problem, AFAICS this is just a classic problem of a) not declaring all the inputs properly, b) not being able to. If we make nix_file
a list attribute instead of a label attribute, then we can provide as many *.nix
source files as we want. If Nixpkgs itself changes, then that will be captured by a change in the definition of nixpkgs_repository
.
So in short, I think this is the right thing to do. But a spec by example should be closer to:
nixpkgs_package(
name = "foo",
srcs = glob(["*.nix"]),
attribute_path = "foo",
repository = "@nixpkgs",
)
@mboes the point of using But what about |
@mboes I don't think the issue is only up to |
@mboes name changed from |
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.
ctx.path(filename) is ineffective in introducing an explicit dependency of the repository rule on filename. Use symlinks instead.
Previously, it was needlessly depending on the network, and depending on large files downloaded from the network.
65b3bac
to
f028a8b
Compare
@Fuuzetsu |
This is a solution for #17, the new
data
attribute references all the filesneeded to execute the nix expression, hence invalidating cache if they are
modified.