-
Notifications
You must be signed in to change notification settings - Fork 45
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
Make Native Link installable via nix #442
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.
+@allada @MarcusSorealheis @adam-singer @blakehatch
The updated readme is more readable at https://github.com/aaronmondal/turbo-cache/tree/nix-package
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @allada)
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.
+@MarcusSorealheis +@adam-singer +@blakehatch
Reviewable status: 0 of 5 files reviewed, all discussions resolved (waiting on @adam-singer, @allada, @blakehatch, and @MarcusSorealheis)
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've already reviewed this change based on the other PR I just reviewed, but I am reserving my overarching comments for this PR. It's very large and proposes a lot of changes. I think we still need to have a way to download the library via Cargo and the Docker container registry.
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.
Not gunna lie, I laughed a lot at the emojis 😆
Reviewed 5 of 5 files at r1, all commit messages.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @aaronmondal, @adam-singer, @blakehatch, and @MarcusSorealheis)
flake.nix
line 45 at r1 (raw file):
native-link = craneLib.buildPackage (commonArgs // {
nit: needless?
README.md
line 7 at r1 (raw file):
Native link is an extremely (blazingly?) fast and efficient build cache and remote executor for systems that communicate using the [Remote execution protocol](https://github.com/bazelbuild/remote-apis/blob/main/build/bazel/remote/execution/v2/remote_execution.proto) such as [Bazel](https://bazel.build), [Buck2](https://buck2.build) and [Goma](https://chromium.googlesource.com/infra/goma/client/).
nit: Maybe add ReClient?
README.md
line 54 at r1 (raw file):
# Unoptimized development build on Windows bazel run --config=windows cas
nit: the Cargo example uses -- ./native-link-config/examples/basic_cas.json
, should we do the same here?
README.md
line 65 at r1 (raw file):
* Cargo 1.73.0+ * A recent C++ toolchain with LLD as linker
Does Cargo need a C++ toolchain? I thought building protoc was the only C++ code required, which is not needed by cargo.
This commit makes Native Link directly buildable/runnable via nix: ```bash nix run github:TraceMachina/native-link /path/to/config.json ``` To reduce closure size for upcoming container images the environment that builds the derivation is a fairly customized LLVM-based toolchain. This toolchain should make it easier to migrate to more sophisticated approaches in the future. Test infrastructure is not yet enabled as it'll be a larger effort to make our testsuite compatible with strictly controlled environments like the nix build sandbox. Importantly, the updated README now makes extensive use of emojis ✨
bb5db4f
to
0f7a228
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.
@MarcusSorealheis Agreed that we want cargo install
and direct docker run
support for images tagged by a semantic version. Both of those require a tagged release. Regarding Containers #443 gives us almost everything except for SemVer tag handling. Regarding Cargo support I'll add another workflow that publishes nlink on crates.io.
@allada 🌱
Reviewable status: 4 of 5 files reviewed, 2 unresolved discussions (waiting on @adam-singer, @allada, @blakehatch, and @MarcusSorealheis)
flake.nix
line 45 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
nit: needless?
For the currently enabled code we could indeed say
native-link = craneLib.buildPackage (commonArgs
// {
cargoArtifacts = craneLib.buildDepsOnly commonArgs;
});
Which also suggests using an immediately invoked lambda:
native-link = (args:
(craneLib.buildPackage (args // {
cargoArtifacts = craneLib.buildDepsOnly args;
})
commonArgs)
But I think the way it's now is fine as it makes it easier to uncomment the tests
block while ensuring that we're always using the same cargoArtifacts
.
The merge operator //
doesn't recurse and inherit x
is the same as saying x = x
. Expanded this code says something like:
native-link = craneLib.buildPackage {
inherit src;
buildInputs = [];
nativeBuildInputs = [ pkgs.autoPatchelfHook pkgs.cacert ];
stdenv = customStdenv;
cargoArtifacts = craneLib.buildDepsOnly {
inherit src;
buildInputs = [ ];
nativeBuildInputs = [ pkgs.autoPatchelfHook pkgs.cacert ];
stdenv = customStdenv;
};
};
Note that buildDepsOnly
and buildPackage
are technically entirely different functions that "just happen" to both accept the commonArgs
attrset as argument.
Without this the two functions would end up using default arguments and for instance use a different stdenv/toolchain for the deps and the native-link-*
crates.
README.md
line 65 at r1 (raw file):
Previously, allada (Nathan (Blaise) Bruer) wrote…
Does Cargo need a C++ toolchain? I thought building protoc was the only C++ code required, which is not needed by cargo.
This is a tricky one. Technically you're correct in that Cargo only depends on the linker but not a full C++ toolchain. However, in most configurations Cargo won't use the linker directly and instead use clang++
or g++
as "wrapped linker" since those are preconfigured to link dynamic dependencies like libc
, libm
and the dynamic linker such asld-linux-x86-64.so.2
.
It's possible to configure Cargo with a "raw" linker and ideally this would be the default behavior. But at the moment this would require users to explicitly configure linking of the non-rust shared objects.
I'll keep this as it is for now and will look into whether we can configure our Cargo build in a way that makes it depend on just a raw linker for all users so that we can safely remove this requirement. Using musl by default likely makes this dependency obsolete as it'll allow us to default to fully static builds where we don't need to worry about distro-specific locations of shared objects.
This commit makes Native Link directly buildable/runnable via nix:
To reduce closure size for upcoming container images the environment that builds the derivation is a fairly customized LLVM-based toolchain. This toolchain should make it easier to migrate to more sophisticated approaches in the future.
Test infrastructure is not yet enabled as it'll be a larger effort to make our testsuite compatible with strictly controlled environments like the nix build sandbox.
Importantly, the updated README now makes extensive use of emojis ✨
This change is