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

Build nativelink with musl #583

Merged
merged 1 commit into from
Jan 1, 2024
Merged

Conversation

aaronmondal
Copy link
Member

@aaronmondal aaronmondal commented Dec 29, 2023

Reduce container image sizes by over 50% to about 28 megabytes.

The nativelink executable is now fully self-contained and portable across linux systems regardless of the host's libc.

This change introduces new Rust toolchains on the nix side that mirror the Bazel toolchains. The new toolchains enable cargo clippy invocations without Bazel and allow invoking the same nightly rustfmt that is used in the Bazel build via pre-commit hooks.


This change is Reviewable

@MarcusSorealheis
Copy link
Collaborator

@blakehatch can you check this on the Mac please?

@MarcusSorealheis
Copy link
Collaborator

I will also, later today sometime

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

@MarcusSorealheis This executable won't work on mac. The statically linked musl still calls into the Linux kernel which Mac doesn't have.

However, this should make it a lot easier to get the same thing working for mac in a future commit since musl does support mac. We'll need a separate build job for that but it should be very close to this linux build.

Reviewable status: 0 of 1 LGTMs obtained

@MarcusSorealheis
Copy link
Collaborator

I don't think we can change this in that case just yet. Or, if we do, we'd need to do a lot to offer a path that does work for users building on a Mac. Many of our users are on Mac OS, certainly from a building from source lens.

Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

Reviewable status: 0 of 1 LGTMs obtained


flake.nix line 54 at r1 (raw file):

        };

        craneLib = crane.lib.${system};

Ah my bad. I thought we only supported the native Cargo build on mac. But we actually did have a working Mac build here that was just not tracked in CI i think. So I definitely should add mac support in this change as well.

Reduce container image sizes by over 50% to about 28 megabytes.

The nativelink executable is now fully self-contained and portable
across linux systems regardless of the host's libc.

This change introduces new Rust toolchains on the nix side that mirror
the Bazel toolchains. The new toolchains enable `cargo clippy`
invocations without Bazel and allow invoking the same nightly `rustfmt`
that is used in the Bazel build via pre-commit hooks.
Copy link
Member Author

@aaronmondal aaronmondal left a comment

Choose a reason for hiding this comment

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

I've added Mac support back. This commit should now be a no-op change for MacOS.

Reviewable status: 0 of 1 LGTMs obtained, and pending CI: Analyze (javascript-typescript), Analyze (python), Bazel Dev / ubuntu-22.04, Cargo Dev / ubuntu-22.04, Local / ubuntu-22.04, Remote / large-ubuntu-22.04, asan / ubuntu-22.04, docker-compose-compiles-nativelink (20.04), docker-compose-compiles-nativelink (22.04), integration-tests (20.04), integration-tests (22.04), pre-commit-checks, publish-image, ubuntu-20.04, ubuntu-20.04 / stable, ubuntu-22.04, ubuntu-22.04 / stable, windows-2022 / stable

Copy link
Contributor

@adam-singer adam-singer left a comment

Choose a reason for hiding this comment

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

:lgtm:

Tested on MacOS

nix develop
nix build
cargo test --all

Reviewed 3 of 4 files at r1, 2 of 2 files at r2, all commit messages.
Reviewable status: 1 of 1 LGTMs obtained

Copy link
Collaborator

@MarcusSorealheis MarcusSorealheis left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: 2 of 1 LGTMs obtained

@MarcusSorealheis MarcusSorealheis removed the request for review from blakehatch December 31, 2023 22:47
@aaronmondal aaronmondal merged commit ee4846c into TraceMachina:main Jan 1, 2024
20 checks passed
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