-
-
Notifications
You must be signed in to change notification settings - Fork 14.6k
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
Reintroducing mkRustCrate #31150
Reintroducing mkRustCrate #31150
Conversation
This is another way to compile Rust programs, which uses Nix instead of Cargo to manage dependencies. Compiling a package can reuse previously compiled dependencies. This uses a companion Rust crate called generate-nix-pkg to produce the .nix files from a Cargo.lock.
I'd love documentation (at least hints?) on two things:
|
Take this for what it's worth, updating https://github.com/NixOS/nixpkgs/blob/master/doc/languages-frameworks/rust.md is probably the right place. (I wasn't aware that markdown was an option when I updated the ruby.xml sibling document...) You can both make it clearer where mkRustCrate lives viz-a-viz the existing alternatives, and explain how to use it. It couldn't hurt to include a use of mkRustCrate in the PR as an example. |
Perhaps a packaged version of |
How can I try it out? I tried by just downloading the main file, and using it locally in my project with:
However I'm getting:
so I'm not sure if it's a right approach. Other than that it seems it currently does not support
and it seems to do the job. With that fix I'm hitting the same problem as above in my packages that required BTW. I've filled some issues about Last, but not least, using this opportunity I'd like to say that @P-E-Meunier , you are my hero. Awesome work and thank you for reddit comments and pijul! |
@dpc, I've seen your issues on the Nest, thanks a lot for them. In order to use mkRustCrate, you need to Example:
This produces a file, |
@P-E-Meunier You mean Oh. So you're saying that I have to run BTW. Back to trying it out. Will report on results. Thanks! |
I think |
Oh, I see. Anyway, I think I got it. I've created a simple nix-shell script to try it out (
It worked beautifully for a moment (dependencies getting build) and then I've hit a problem with
but I guess it's a problem with |
@dpc I've just pushed a new commit to handle that case. |
echo "$boldgreen" "Building $BUILD (${libName})" "$norm" | ||
mkdir -p target/build/${crateName} | ||
if [ -e target/link_ ]; then | ||
rustc --crate-name build_script_build $BUILD --crate-type bin ${rustcOpts} ${crateFeatures} --out-dir target/build/${crateName} --emit=dep-info,link -L dependency=target/deps ${deps} --cap-lints allow $(cat target/link_) |
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.
Can you wrap long lines like this for readability?
mkdir -p $OUT_DIR | ||
target/build/${crateName}/build_script_build > target/build/${crateName}.opt | ||
set +e | ||
EXTRA_BUILD=$(grep "^cargo:rustc-flags=" target/build/${crateName}.opt | sed -e "s/cargo:rustc-flags=\(.*\)/\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.
Why is both grep
and sed
required here?
norm="$(printf '\033[0m')" #returns to "normal" | ||
bold="$(printf '\033[0;1m')" #set bold | ||
green="$(printf '\033[0;32m')" #set red | ||
boldgreen="$(printf '\033[0;1;32m')" #set bold, and set red. |
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 is better use tput
here. Otherwise it might print garbage if stdout is not a terminal.
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.
I'm trying to do this, but I keep getting the error
tput: No value for $TERM and no -T specified
When I set a TERM to my terminal, I get instead:
tput: terminal attributes: No such device or address
The command I'm trying to use is
echo "`tput setaf 2``tput bold`Building $BUILD (${libName})`tput sgr0`"
|
||
let buildCrate = { crateName, crateVersion, dependencies, complete, crateFeatures, libName, build, release, libPath, crateType, metadata, crateBin, finalBins, verboseBuild }: | ||
|
||
let depsDir = builtins.foldl' (deps: dep: deps + " " + dep.out) "" dependencies; |
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.
concatStringsSep " " dependencies
let buildCrate = { crateName, crateVersion, dependencies, complete, crateFeatures, libName, build, release, libPath, crateType, metadata, crateBin, finalBins, verboseBuild }: | ||
|
||
let depsDir = builtins.foldl' (deps: dep: deps + " " + dep.out) "" dependencies; | ||
completeDepsDir = builtins.foldl' (deps: dep: deps + " " + dep.out) "" complete; |
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.
same as above.
" --extern ${extern}=${dep.out}/lib${extern}-${dep.metadata}.rlib" | ||
else | ||
" --extern ${extern}=${dep.out}/lib${extern}-${dep.metadata}.so") | ||
) "" dependencies; |
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.
foldl'
can be here also replaced by concatMapStringsSep " " (dep: ...) dependencies
) "" dependencies; | ||
optLevel = if release then 3 else 0; | ||
rustcOpts = (if release then "-C opt-level=3" else "-C debuginfo=2"); | ||
rustcMeta = "-C metadata=" + metadata + " -C extra-filename=-" + metadata; |
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.
rustcMeta = "-C metadata=${metadata} -C extra-filename=-${metadata}";
complete = builtins.foldl' (comp: dep: if lib.lists.any (x: x == comp) dep.complete then comp ++ dep.complete else comp) dependencies dependencies; | ||
|
||
crateFeatures = if crate ? features then | ||
builtins.foldl' (features: f: features + " --cfg feature=\\\"${f}\\\"") "" crate.features |
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.
Can be also replaced by concatStringsSep
.
export CARGO_PKG_NAME=${crateName} | ||
export CARGO_PKG_VERSION=${crateVersion} | ||
export CARGO_CFG_TARGET_ARCH=$(echo ${buildPlatform.system} | sed -e "s/\([^-]*\)-\([^-]*\)/\1/") | ||
export CARGO_CFG_TARGET_OS=$(echo ${buildPlatform.system} | sed -e "s/\([^-]*\)-\([^-]*\)/\2/") |
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.
buildPlatform.parsed.kernel.name
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.
Where is the documentation for this? If it doesn't exist, I'd like to add it somewhere.
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.
I found out, while playing the nix-repl
:
$ nix-repl
Welcome to Nix version 1.11.15. Type :? for help.
nix-repl> :l <nixpkgs>
Added 7885 variables.
nix-repl> buildPlatform.parsed
{ _type = "system"; abi = { ... }; cpu = { ... }; kernel = { ... }; vendor = { ... }; }
nix-repl>buildPlatform.parsed.kernel
{ _type = "kernel"; execFormat = { ... }; families = { ... }; name = "linux"; }
I think this was merged recently for cross compiling. It comes from lib/systems
.
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 might belong to the nixpkgs documentation then, located in doc
. If you are in hurry we also have a wiki to braindump knowledge: https://nixos.wiki/
BUILD_OUT_DIR="" | ||
export CARGO_PKG_NAME=${crateName} | ||
export CARGO_PKG_VERSION=${crateVersion} | ||
export CARGO_CFG_TARGET_ARCH=$(echo ${buildPlatform.system} | sed -e "s/\([^-]*\)-\([^-]*\)/\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.
buildPlatform.parsed.cpu.name
export CARGO_MANIFEST_DIR="." | ||
export DEBUG="${toString (!release)}" | ||
export OPT_LEVEL="${toString optLevel}" | ||
export TARGET="${buildPlatform.system}-gnu" |
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.
Maybe buildPlatform.config
? I have: "x86_64-unknown-linux-gnu"
Wow, that's a serious review. Thanks a lot! |
@P-E-Meunier sorry, this was slacked too long on my todo list. |
cc @Ericson2314 I guess cross-compilation can be also added later, but maybe you have an opinion on the variables that are passed in. |
export DEBUG="${toString (!release)}" | ||
export OPT_LEVEL="${toString optLevel}" | ||
export TARGET="${buildPlatform.system}-gnu" | ||
export HOST="${buildPlatform.system}-gnu" |
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.
buildPlatform.config
These variables will change later for cross compilation.
doc/languages-frameworks/rust.md
Outdated
hello_0_1_0.override { crateOverrides = overrides; } | ||
``` | ||
|
||
Here, `crateOverrides` is expected to be a attribute set, where the |
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.
Nit: "an attribute set"
|
||
{ | ||
libsqlite3-sys = attrs: { buildInputs = [ pkgconfig sqlite ] ++ (attrs.buildInputs or []); }; | ||
openssl-sys = attrs: { buildInputs = [ pkgconfig openssl ] ++ (attrs.buildInputs or []); }; |
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.
Whatever format we start with in this file will be likely followed blindly until it gets way out of hand. Can you reformat these to be multiple lines? Example:
libsqlite3-sys = attrs: {
buildInputs = [ pkgconfig sqlite ]
++ (attrs.buildInputs or []);
};
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 single line looks more amenable to reading imho, easy to copy/paste a line, move it up and down in alphabetical order using an editor. Also this file could easily get big much like all-packages.nix
, so keeping it one liners helps. 2cw
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.
I will not become this big. Most package won't need overriding.
Our defaultGemConfig
file is only 342 lines long. Haskell overrides are in the same size magnitude. Having a long single line makes diffs hard to read.
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.
well okay then, if other language level pkg managers use a similar indentation convention then best to stick with it, but at least do this after the merge?
version = lib.splitString "." (lib.head version_); | ||
authors = lib.concatStringsSep ":" crateAuthors; | ||
in '' | ||
norm="" |
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.
I think it is a bit scary to have a giant set of complex shell scripts embedded in to a .nix file. What would it take to move the shell scripts in to dedicated files, and reference them from here? Then, we can easily use static analysis to help improve / understand them later on.
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.
I tend to agree, but the shell script uses lots of parameters that are nix variables, and some parts are even of the form ${optionalString …}
. I guess the way to separate that script would be to create bash functions, but these are harder to read and get right than the Nix code we currently use.
Since I've never done it before, this might as well come from a big misunderstanding of what you meant, so please correct me if I'm wrong.
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.
I agree with @P-E-Meunier that we by not embedding shell in nix we loose useful nix features.
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.
buildCrate
could become its own nix file though.
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.
I'm working on a patch to split the shell script into a separate file. Not all of the shell code will transfer, but I think I can get most, and reduce the nix file to just initializing env vars, sourcing the shell script, and running a start_build()
shell function or something. At the very least, it'll be something concrete to review and compare.
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.
Okay. Initial patch is drafted. The resulting rust-utils.nix
[1] and shell script [2] are available for viewing.
[1]: https://nest.pijul.com/boxofrox/nix-rust:master/8ce71bef4766c2439602
[2]: https://nest.pijul.com/boxofrox/nix-rust:master/289f899c9c9f6d041d
Or if nest is down, here's a gist [3] with the two files.
[3]: https://gist.github.com/boxofrox/7285293d9a552dbf98c421f41838e7ea
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.
@P-E-Meunier, @boxofrox's work looks great, are you keen to use it?
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.
Looking pretty good to me. I'm giving it a test with ofborg and will file my feedback on that shortly. I'm a bit concerned about the big shell script being inside the .nix file, that makes it pretty hard to grok and edit later. If possible, I'd like to see that moved to its own file.
When Ibuild carnix, I now get weird output:
|
pkgs/build-support/rust/carnix.nix
Outdated
backtrace_sys_0_1_16 = backtrace_sys_0_1_16_ { | ||
++ (if kernel == "windows" then [ dbghelp_sys_0_2_0 kernel32_sys_0_2_2 winapi_0_2_8 ] | ||
++ (if lib.lists.any (x: x == "dbghelp-sys") features then [dbghelp_sys_0_2_0] else []) ++ (if lib.lists.any (x: x == "kernel32-sys") features then [kernel32_sys_0_2_2] else []) ++ (if lib.lists.any (x: x == "winapi") features then [winapi_0_2_8] else []) else []); | ||
features = mkFeatures backtrace_0_3_4_features; |
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.
I have not yet understood the purpose of this function. What does it compute exactly?
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.
Previously, Carnix was resolving the dependencies itself. This is not only redundant with what Nix can do, it can also become wrong when some features of some dependencies are enabled only on some platforms (a problem detected by @grahamc).
I'm now using the following trick to resolve features: each package A
that wants feature f from dependency B
adds the following line to the "function calls" part of the file:
B."f".from_A = true;
Then, mkFeatures
aggregates all such declarations and decides whether "f" should be enabled on package B. Sometimes, feature requests can get more complicated and include if
statements about the platform, or the activation of another feature.
The "default" feature is trickier, since it is enabled if either no dependent package mentions it, or at least one doesn't want "use-default-features".
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.
Is this equivalent to the following code?
{
crate_a = buildRustCrate {
features = ["foo"]; # if "foo" would be a default feature
};
crate_b = {
dependencies = [
(crate_a.override { features = ["foo" "bar"];} ) # if crate_b has requested "bar" in addition
];
};
}
Should we merge this and continue working on it in master? It would at least make contributions by others than @P-E-Meunier easier. |
I'm ok with merging. |
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.
I'd say we merge it.
@P-E-Meunier can we do some 1:1 time on IRC to sort out some issues I'm having with this? I'd like to get it merged soon. |
Could we at least merge this now so that others can start work on it? |
Kudos @P-E-Meunier - thank you. |
Is there a working link to an issue tracker for carnix? https://crates.io/crates/carnix links to https://nest.pijul.com/pmeunier/carnix which gives me a "Not found". |
The link is actually https://nest.pijul.com/pmeunier/nix-rust |
Thank you, everyone, for getting this reviewed and merged. Thank you, @P-E-Meunier for authoring! |
* bemenu: init at 2017-02-14 * velox: 2015-11-03 -> 2017-07-04 * orbment, velox: don't expose subprojects the development of orbment and velox got stuck their subprojects (bemenu, dmenu-wayland, st-wayland) don't work correctly outside of parent projects so hide them to not confuse people swc and wld libraries are unpopular and unlike wlc are not used by anything except velox * pythonPackages.pydbus: init at 0.6.0 * way-cooler: 0.5.2 -> 0.6.2 * nixos/way-cooler: add module * dconf module: use for wayland non-invasive approach for #31293 see discussion at #32210 * sway: embed LD_LIBRARY_PATH for #32755 * way-cooler: switch from buildRustPackage to buildRustCrate #31150
Motivation for this change
This is another way to compile Rust programs, which uses Nix instead
of Cargo to manage dependencies. Compiling a package can reuse
previously compiled dependencies.
This uses a companion Rust crate called generate-nix-pkg to produce
the .nix files from a Cargo.lock.
Especially useful in combination with NixOps.
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)