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

Variation of #214 (Rule to generate docker images with nix store paths) #351

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

dmadisetti
Copy link
Contributor

Reduced change set of #214, with motivation of getting some momentum behind this.

Notable changes, utilizing the bazel-support directory to store default.nix and BUILD files so the docker definition can hook in.

TODO:

  • Tests
  • Documentation
  • Allow for build artifacts to be bundled

Ideally, I'm going to use this as starting point so I can get singularity image builds more seamlessly in my workflow as an approach to #291

@dmadisetti
Copy link
Contributor Author

@jcpetruzza @aherrmann

@benradf
Copy link
Member

benradf commented Mar 16, 2023

Looks like the CI errors are due to the tilde in path issue that @aherrmann encountered a little while ago.

@dmadisetti
Copy link
Contributor Author

Oh interesting. Is this the work around? NixOS/nix#7742 (comment)

I'll take a stab at this later today

@benradf
Copy link
Member

benradf commented Mar 16, 2023

Oh interesting. Is this the work around? NixOS/nix#7742 (comment)

I'll take a stab at this later today

Yeah, I think that should do the trick. From the documentation for nixpkgs_package:

Note, labels to external workspaces will resolve to paths that contain ~ characters if the Bazel flag --enable_bzlmod is true. Nix does not support ~ characters in path literals at the time of writing, see #7742. Meaning, the result of location expansion may not form a valid Nix path literal. Use ./$${"$(location @for//:example)"} to work around this limitation if you need to pass a path argument via --arg, or pass the resulting path as a string value using --argstr and combine it with an additional --arg workspace_root ./. argument using workspace_root + ("/" + path_str).

@@ -0,0 +1,15 @@
package(default_visibility = ["//visibility:public"])
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

One problem is the unquoted ~ as discussed above. The linked workaround is the correct fix.

Another issue with bzlmod mode will be that this containers package doesn't fit into any of the new rules_nixpkgs Bazel modules (see #181, in particular #182 and #322). As laid out there rules_nixpkgs has been split into multiple sub-modules such as rules_nixpkgs_core under core/ or rules_nixpkgs_cc under toolchains/cc.

The motivation for this split is to avoid rules_nixpkgs turning into a Bazel module that transitively depends on almost all Bazel modules out there.

For the specific rule added here, it doesn't actually introduce any new Bazel module dependencies. The fact that it generates a Docker image might suggest that it should belong into a dedicated rules_nixpkgs_containers or rules_nixpkgs_docker Bazel module. But, since it only depends on functionality in rules_nixpkgs_core and Nix itself, it can actually be part of rules_nixpkgs_core.

@benradf any thoughts on this? My inclination would be to go with the simpler route of just placing this into core/ intead of introducing all the overhead of a new dedicated rules_nixpkgs_containers module when it's not really necessary.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@benradf any thoughts on this? My inclination would be to go with the simpler route of just placing this into core/ intead of introducing all the overhead of a new dedicated rules_nixpkgs_containers module when it's not really necessary.

I agree, there's no compelling reason for a new module right now, so putting it in rules_nixpkgs_core makes most sense. It can always be moved later if additional module dependencies are required.

@dmadisetti dmadisetti force-pushed the docker branch 2 times, most recently from 0dccf1d to 72ecd4d Compare April 4, 2023 02:53
@dmadisetti
Copy link
Contributor Author

Sorry for the failure spam. I was unable to replicate this locally + found additional failure cases I wasn't seeing locally.

If this doesn't work, I will revisit later

@dmadisetti
Copy link
Contributor Author

Yeah. No idea. Running bazel test tests:all from testing/core passes locally, and I have no weird _main~non_module_deps~hello external dep (tried bazel_5 and bazel_6). Also seems to work executing .github/build-and-test. I guess I don't really understand how these special paths are arising?

@aherrmann
Copy link
Member

I guess I don't really understand how these special paths are arising?

They should arise when you enable bzlmod either by bazel test --config=bzlmod in testing/core, or with export BZLMOD_ENABLED=true when running .github/build-and-test.

@benradf
Copy link
Member

benradf commented Sep 7, 2023

I'd like to try and get this merged. Unfortunately this commit that refactored _nixpkgs_package_impl has caused a non-trivial merge conflict. I'll have a go at resolving it when I have some time though.

@dmadisetti
Copy link
Contributor Author

I've been using an extension of this that packages build artifacts: https://github.com/cemel-jhu/rules_nixpkgs/pull/1/files

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

Successfully merging this pull request may close these issues.

3 participants