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: add skip of sub-build on abi generation to circumvent cargo-near#287 #212

Open
wants to merge 3 commits into
base: refactor/upgraded-reproducible-config
Choose a base branch
from

Conversation

dj8yfo
Copy link

@dj8yfo dj8yfo commented Jan 13, 2025

near/cargo-near#287 is indeed a bug,
fix in near/cargo-near#289 will be published with cargo-near-build crate some time later,

while this pr circumvents the issue and also saves on some build time by not running nested build during current contract's abi build

relevant lines are

            // circumvents the issue in 287
            
            .build_skipped_when_env_is(vec![
                // shorter build for `cargo check`
                ("PROFILE", "debug"),
                (cargo_near_build::env_keys::BUILD_RS_ABI_STEP_HINT, "true"),
            ])

and

        // unix path to target sub-contract's crate from root of the repo
        let nep330_contract_path = "web4/treasury-web4";

and rest is just puff

@dj8yfo dj8yfo changed the title chore: add skip of sub-build on abi generation to circumvent https://github.com/near/cargo-near/issues/287 chore: add skip of sub-build on abi generation to circumvent cargo-near#287 Jan 13, 2025
@dj8yfo dj8yfo marked this pull request as ready for review January 13, 2025 20:28
Copy link
Contributor

@frol frol left a comment

Choose a reason for hiding this comment

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

@dj8yfo Does it have to be this complex? Is there a way to improve cargo-near-build to hide this whole complexity there?

treasury-factory/build.rs Outdated Show resolved Hide resolved
treasury-factory/build.rs Show resolved Hide resolved
@dj8yfo
Copy link
Author

dj8yfo commented Jan 14, 2025

@frol there's a plan for a task to compute

// unix path to target sub-contract's crate from root of the repo
        let nep330_contract_path = "web4/treasury-web4";

automatically, the rest is pretty much what current documentation snippet suggests (i.e. has been accepted previously).
I've added more output about additional pipeline stage with base64 encoding, so that it's present in console for whoever builds this project for their first time for easier introspection. Not sure if we want to consider this additional stage default functionality of cargo-near-build.

treasury-factory/build.rs Outdated Show resolved Hide resolved
("PROFILE", "debug"),
(cargo_near_build::env_keys::BUILD_RS_ABI_STEP_HINT, "true"),
])
.stub_path(ChildContract::STUB_PATH)
Copy link
Author

Choose a reason for hiding this comment

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

stub_path can probably be computed automatically and not be an option on builder, needed to be configured

Copy link
Author

Choose a reason for hiding this comment

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

good candidate for this looks to be

    fn sub_build_stub_result_path(sub_build_crate_name: &str) -> String {
        let out_dir_env = std::env::var("OUT_DIR").expect("OUT_DIR is always set in build scripts");
        let out_dir = Path::new(&out_dir_env);

        let path = out_dir.join(format!("{}-stub.bin", sub_build_crate_name));
        path.to_str().expect("valid unicode").to_owned()
    }

which is

warning: [email protected]: Build empty artifact stub-file written to: `/home/user/Documents/code/neardevhub-treasury-dashboard/treasury-factory/target/debug/build/treasury-factory-566aa3a029070e78/out/treasury-web4-stub.bin`

as a result

.manifest_path("../web4/treasury-web4/Cargo.toml".into())
.manifest_path(child_contract.manifest)
.override_nep330_contract_path(child_contract.nep330_contract_path)
.override_cargo_target_dir(ChildContract::sub_build_target_dir())
Copy link
Author

@dj8yfo dj8yfo Jan 14, 2025

Choose a reason for hiding this comment

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

override_cargo_target_dir can probably be made computed automatically too and attempted to be removed from public api, but that has to happen after nep330_contract_path and stub_path iterations

Copy link
Author

Choose a reason for hiding this comment

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

Copy link
Author

@dj8yfo dj8yfo Jan 14, 2025

Choose a reason for hiding this comment

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

good candidate for this looks to be

    fn sub_build_target_dir(sub_build_crate_name: &str, current_crate_name: &str) -> String {
        let out_dir_env = std::env::var("OUT_DIR").expect("OUT_DIR is always set in build scripts");
        let out_dir = Path::new(&out_dir_env);

        let path = out_dir.join(format!(
            "target-{}-for-{}",
            sub_build_crate_name, current_crate_name
        ));

        path.to_str()
            .unwrap_or_else(|| panic!("valid unicode path expected {}", path.to_string_lossy()))
            .to_owned()
    }

which results in

warning: [email protected]: Build artifact path: /home/jerrick/Documents/code/neardevhub-treasury-dashboard/treasury-factory/target/wasm32-unknown-unknown/release/build/treasury-factory-2b4e19c8a0b12f8f/out/target-treasury-web4-for-treasury-factory/near/treasury_web4.wasm

Copy link
Author

@dj8yfo dj8yfo Jan 14, 2025

Choose a reason for hiding this comment

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

this has to be tested with how this principle looks on more than 1 level of nesting in near-devhub contract

(comment with an output of tested nested build-scripts pending in this thread)

Copy link
Author

Choose a reason for hiding this comment

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

it looks like above sub_build_target_dir fn is a good default for an override_cargo_target_dir option:

  • community built
    • wasm : target/near/devhub_community/devhub_community.wasm
    • sub-build
      • stub : target/debug/build/devhub-community-9df08e869c916c9b/out/discussions-stub.bin
      • wasm : target/wasm32-unknown-unknown/release/build/devhub-community-1515b1be94ff0e90/out/target-discussions-for-community/near/devhub_discussions/devhub_discussions.wasm
  • community-factory built
    • wasm : target/near/devhub_community_factory/devhub_community_factory.wasm
    • sub-build
      • stub : target/debug/build/devhub-community-factory-d7fa895f926988cc/out/community-stub.bin
      • wasm : target/wasm32-unknown-unknown/release/build/devhub-community-factory-25fc80ffdbb5bdec/out/target-community-for-community-factory/near/devhub_community/devhub_community.wasm
      • sub-build
        • stub : target/wasm32-unknown-unknown/release/build/devhub-community-factory-25fc80ffdbb5bdec/out/target-community-for-community-factory/debug/build/devhub-community-9df08e869c916c9b/out/discussions-stub.bin
        • wasm : target/wasm32-unknown-unknown/release/build/devhub-community-factory-25fc80ffdbb5bdec/out/target-community-for-community-factory/wasm32-unknown-unknown/release/build/devhub-community-1515b1be94ff0e90/out/target-discussions-for-community/near/devhub_discussions/devhub_discussions.wasm

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.

2 participants