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
crystal: reduce closure size, more robust runtime #74682
Conversation
--suffix CRYSTAL_PATH : lib:$out/lib/crystal \ | ||
--suffix LIBRARY_PATH : ${lib.makeLibraryPath buildInputs} | ||
--suffix CRYSTAL_LIBRARY_PATH : ${lib.makeLibraryPath (compiler.buildInputs ++ [ |
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.
how about a:
(compilter.buildInputs ++ (map lib.getLib [ pcre zlib .... ])
to avoid having to hardcode the output?
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.
Ah, I didn't know about this. I tried the make*Path
functions, but none really fit well.
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.
OK, had to work around buildInputs
turning into a list of dev
outputs, so I moved that list outside. It seems a lot cleaner now.
generic = { version, sha256, binary, doCheck ? true }: | ||
let compiler = stdenv.mkDerivation rec { | ||
generic = ({ version, sha256, binary, doCheck ? true, extraBuildInputs ? [] }: | ||
lib.fix (compiler: stdenv.mkDerivation { |
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.
Do you have a reference to documentation for lib.fix?
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.
it's documented at https://github.com/NixOS/nixpkgs/blob/master/lib/fixed-points.nix#L3-L25
@@ -95,16 +96,27 @@ let | |||
"--single-module" # needed for deterministic builds | |||
]; | |||
|
|||
# This makes sure we don't keep depending on the previous version of | |||
# crystal used to build this one. | |||
CRYSTAL_LIBRARY_PATH = "${placeholder "out"}/lib/crystal"; |
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.
Nice catch!!
d373eec
to
bf4b82c
Compare
bf4b82c
to
aa2c03f
Compare
--suffix CRYSTAL_PATH : lib:$out/lib/crystal \ | ||
--suffix LIBRARY_PATH : ${lib.makeLibraryPath buildInputs} | ||
--suffix CRYSTAL_LIBRARY_PATH : ${ | ||
lib.makeLibraryPath (commonBuildInputs extraBuildInputs) |
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.
commonBuildInputs already contains extraBuildInputs.
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.
yes, as an argument, that comes from here.
Motivation for this change
Crystal depended on the previous version after installation, as well as llvm, causing up to 3-4 versions of crystals to be installed at once.
The fix is to set the
CRYSTAL_LIBRARY_PATH
during build, to avoid reusing the one of the previous version which would be baked into the binary and cause Nix to deem it a dependency.This also adds changes to the
CRYSTAL_LIBRARY_PATH
of the wrappedcrystal
binary, to provide needed dependencies for the standard library to avoid common issues that would require a separate nix-shell or derivation. It increases closure size a little, but should overall provide a much nicer user experience and for me it's worth it.Also adding a snapshot of the upcoming
0.32
version, which has a new dependency onreadline
to showcase how we need to make thegeneric
more configurable. It's not exposed on top-level for now, and shouldn't cause any build on its own. But I'm using it because of some new features and will bump it next week when the final release is out.Another change is that I'm using
lib.fix
instead oflet
. This allows overrides to work as expected again, while keeping the ability to pass itself tobuildCrystalPackage
.Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nix-review --run "nix-review wip"
./result/bin/
)nix path-info -S
before and after)Notify maintainers
cc @peterhoeg @david50407