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

chore(cargo-build-sbf): allow building from nix store #4061

Merged
merged 8 commits into from
Dec 16, 2024

Conversation

dzmitry-lahoda
Copy link

@dzmitry-lahoda dzmitry-lahoda commented Dec 11, 2024

Problem

Previously I enabled using sbf compiler from nix and downloaded via nix #3762,
but still had to side effect into HOME to allow sbf builds.

Summary of Changes

In this PR I fully eliminate any side effects to HOME,
following nix guidance not to side effect to HOME and also allowed build programs with nix cache.

If no --no-rustup option provided, all works a before.

cargo run -- --skip-tools-install --no-rustup --sbf-sdk=/nix/store/zmr52j81db288nrg7arvn98-whatever --tools-version=v1.43

@mergify mergify bot requested a review from a team December 11, 2024 16:48
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for your contribution again! I'm a fan of the change, but I want to be sure that I understand how to use it.

If you pass --no-rustup, then for it to work, you must either set RUSTC, ie. RUSTC=/path/to/solana/rustc cargo build-sbf..., or ensure that the default rustc in your PATH is the Solana version of rustc. Is that correct?

Otherwise, just a few nits, it looks good to me

sdk/cargo-build-sbf/src/main.rs Outdated Show resolved Hide resolved
sdk/cargo-build-sbf/src/main.rs Outdated Show resolved Hide resolved
sdk/cargo-build-sbf/src/main.rs Outdated Show resolved Hide resolved
@@ -1048,6 +1066,13 @@ fn main() {
.takes_value(false)

Choose a reason for hiding this comment

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

I can't comment higher up, but let rust_base_version = get_base_rust_version(...) on line 995 will use homedir to find the tools version. Since this is just for logging with CI caches, I assume it's OK for it to fail and return an empty string, but I wanted to flag it either way.

Copy link
Author

@dzmitry-lahoda dzmitry-lahoda Dec 13, 2024

Choose a reason for hiding this comment

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

    let rust_base_version = get_base_rust_version(DEFAULT_PLATFORM_TOOLS_VERSION);
    let version = format!(
        "{}\nplatform-tools {}\n{}",
        crate_version!(),
        DEFAULT_PLATFORM_TOOLS_VERSION,
        rust_base_version,
    );
    let matches = clap::Command::new(crate_name!())
        .about(crate_description!())
        .version(version.as_str())

Oh, recalled what problem was.

  1. I can invoke cargo build sbf in nix even if this line runs. No worries.
  2. I cannot condition out these not to run with nix. Because cli parameters not yet parsed. Until i change usage of clap and provide own --version command. I want to avoid doing anything .version(version.as_str()) in this PR. But seems solution can be backward compatible and provide correct tools and rust version.

Problem also seems exists without nix, if custom sbf path/tools-version provided. Fix is custom --version --WHATEVER_PARAMETERS_OVERRIDE_RUST.

Choose a reason for hiding this comment

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

Ok great, just wanted to make sure everything is good for your use case

@dzmitry-lahoda
Copy link
Author

dzmitry-lahoda commented Dec 12, 2024

if you pass --no-rustup, then for it to work, you must either set RUSTC, ie. RUSTC=/path/to/solana/rustc cargo build-sbf..., or ensure that the default rustc in your PATH is the Solana version of rustc. Is that correct?

yes. and that is what exactly nix does well.

  1. nix consumer of cargo-build-sbf will wrap it https://github.com/arijoon/solana-nix/blob/52dbde3e8bb09fbee644c371de930d5374c3cb8c/solana-cli.nix#L103 with whatever parameters.
  2. it will use default platforms tools for agave release, but allow override with https://ryantm.github.io/nixpkgs/using/overrides/ . from this override platforms tool will be taken and relevant cargo/rust
  3. final consumer can override platform-tools as he pleased too.

specifically i just upstreaming this patch https://github.com/arijoon/solana-nix/blob/52dbde3e8bb09fbee644c371de930d5374c3cb8c/cargo-build-sbf.patch so that nix and non nix (default) solution work from same release

@joncinque joncinque added the CI Pull Request is ready to enter CI label Dec 16, 2024
@anza-team anza-team removed the CI Pull Request is ready to enter CI label Dec 16, 2024
Copy link

@joncinque joncinque left a comment

Choose a reason for hiding this comment

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

Thanks for all the info, I'm planning on learning more about Nix over the coming weeks, so these resources are a big help.

Either way, the change looks good! Once it passes CI, I'll merge it in.

@joncinque joncinque merged commit eec244f into anza-xyz:master Dec 16, 2024
19 checks passed
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.

3 participants