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

pass --target-dir with cargo/build pipeline #1592

Conversation

kranurag7
Copy link
Contributor

@kranurag7 kranurag7 commented Oct 23, 2024

this patch passes --target-dir flag with cargo at build time.

This is required because cargo/build pipeline uses modroot and when we cd into a specific directory and invoke cargo build, the resulting binary still ends up in root of the repository at GIT_ROOT_DIR/target/release/foo

After which when we use the following install command it fails to find the binary.

OUTPUT_PATH="target/release"
install -Dm755 "${OUTPUT_PATH}/${{inputs.output}}" "${INSTALL_PATH}/${{inputs.output}}"

by passing --target-dir flag to target, we are creating the target/release/foo binary in the current working directory hence the install command will succeed.

This change is looking good considering backwards compatibility with the use of this pipeline in existing package.

⚠️ Alternative consideration

We change the existing modroot name to manifest-path and invoke all the commands from the root of the repository.

we can keep using the word modroot as well but IMHO manifest-path closely aligns with cargo terminology.

Manifest Options:
      --manifest-path <PATH>  Path to Cargo.toml

We then need to point this to Cargo.toml in different directory and the result will always be stored at root of the repository inside target/release or target/debug directory depending on the profile we are building for.

To me, this looks like a more better solution than the proposed one but this will break one existing package because of name change https://github.com/wolfi-dev/os/blob/main/geckodriver.yaml

I proceeded with first option because the alternative consideration was breaking one package.

related: #1590

@kranurag7 kranurag7 force-pushed the kr/add-build-base-to-cargo-build-pipeline branch 2 times, most recently from 2912665 to 8b32a2c Compare October 23, 2024 15:09
@kranurag7 kranurag7 force-pushed the kr/add-build-base-to-cargo-build-pipeline branch from 8b32a2c to 7676bde Compare October 23, 2024 15:10
@imjasonh imjasonh merged commit 1cbadb0 into chainguard-dev:main Oct 23, 2024
36 checks passed
@kranurag7 kranurag7 deleted the kr/add-build-base-to-cargo-build-pipeline branch October 23, 2024 17:08
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