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

zz: install modules shipped with compiler #110771

Merged
merged 2 commits into from Jan 27, 2021
Merged

Conversation

sternenseemann
Copy link
Member

Motivation for this change

Previously zz wouldn't be able to compile anything using standard
modules like mem or log out of the box.

To fix this we copy the modules directory included in the source to
$out/share/zz/modules and add an entry to ZZ_MODULE_PATHS in the wrapper
around zz.

We also remove a search path entry which used to reference /build
because it used CARGO_MANIFEST_DIR at build time. The default search
path now includes:

  • /nix/store/modules
  • $out/share/zz/modules
  • $(pwd)/modules

Patching out /nix/store/modules would be kind of cumbersome as it is a
multi-line entry, but it probably does no harm and fine to leave in.

An issue arising by this PR might be that the added search path entry
may take priority over an user specified location even though we use
--suffix. This is because zz internally uses a HashSet which has no
guaranteed iteration order. This may lead to unexpected behavior for
users wo previously provided custom versions of the standard modules via
ZZ_MODULE_PATHS. However, this is an issue in upstream issue as well
where ZZ_MODULE_PATHS may or may not take priority over the compiled in
search path, so this issue should probably be resolved upstream (I'll
file a report or PR).

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

Previously zz wouldn't be able to compile anything using standard
modules like mem or log out of the box.

To fix this we copy the modules directory included in the source to
$out/share/zz/modules and add an entry to ZZ_MODULE_PATHS in the wrapper
around zz.

We also remove a search path entry which used to reference /build
because it used CARGO_MANIFEST_DIR at build time. The default search
path now includes:

* /nix/store/modules
* $out/share/zz/modules
* $(pwd)/modules

Patching out /nix/store/modules would be kind of cumbersome as it is a
multi-line entry, but it probably does no harm and fine to leave in.

An issue arising by this PR might be that the added search path entry
may take priority over an user specified location even though we use
--suffix. This is because zz internally uses a HashSet which has no
guaranteed iteration order. This may lead to unexpected behavior for
users wo previously provided custom versions of the standard modules via
ZZ_MODULE_PATHS. However, this is an issue in upstream issue as well
where ZZ_MODULE_PATHS may or may not take priority over the compiled in
search path, so this issue should probably be resolved upstream (I'll
file a report or PR).
Copy link
Contributor

@marsam marsam left a comment

Choose a reason for hiding this comment

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

Thank you!

@marsam marsam merged commit 9c5389e into NixOS:master Jan 27, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants