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

feat: extra build artifacts #613

Merged
merged 3 commits into from
Nov 29, 2023
Merged

feat: extra build artifacts #613

merged 3 commits into from
Nov 29, 2023

Conversation

mistydemeo
Copy link
Contributor

@mistydemeo mistydemeo commented Nov 28, 2023

This adds support for specifying arbitrary extra artifacts alongside the primary builds. These artifacts are currently expected to be architecture-independent and are uploaded as their own build artifacts rather than bundled into release tarballs. For example, we can use this to replace our current append-release code in to generate and upload the dist-manifest-schema.json.

I realized that most of our generic build code is very well-suited to this, so I split the existing generic builds into half. The code that runs the builds is now shared between this and the generic build path, while another section is separate for implementation detail reasons. There are two main distinctions between these and generic builds:

  • Generic builds are oriented around binaries, which will eventually be collated into an artifact. Extra artifacts produce artifacts directly, and have no associated binaries.
  • Generic builds (usually) build native code and have target triples; as a result, we get N copies of the generic build, one per desired triple. Extra artifacts do not have target triples, and we produce a single copy per release.

The syntax looks like this:

[[workspace.metadata.dist.extra-artifacts]]
artifacts = ["dist-manifest-schema.json"]
build = ["cargo", "dist", "manifest-schema", "--output=dist-manifest-schema.json"]

There can be arbitrary numbers of extra artifacts per build, so this is repeatable within a Cargo.toml/dist.toml as many times as needed.

Closes #349.

@mistydemeo mistydemeo force-pushed the extra-artifacts branch 7 times, most recently from 79a01bd to dd0890e Compare November 28, 2023 18:37
Copy link
Member

@Gankra Gankra left a comment

Choose a reason for hiding this comment

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

Seems right, just a question of the scope of the config

@@ -59,7 +59,7 @@ jobs:
with:
submodules: recursive
- name: Install cargo-dist
run: "curl --proto '=https' --tlsv1.2 -LsSf https:/axodotdev/cargo-dist/releases/download/v0.5.0/cargo-dist-installer.sh | sh"
run: "cargo install --git https:/axodotdev/cargo-dist/ --branch=extra-artifacts cargo-dist"
Copy link
Member

Choose a reason for hiding this comment

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

oroboros....

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Haha, yeah, just for testing. I'm removing this commit before merging.

cargo-dist/src/cli.rs Show resolved Hide resolved
Comment on lines 406 to 408
if extra_artifacts.is_some() {
warn!("package.metadata.dist.extra_artifacts is set, but this is only accepted in workspace.metadata (value is being ignored): {}", package_manifest_path);
}
Copy link
Member

Choose a reason for hiding this comment

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

oh interesting, i would have assumed this was package-specific (vaguely recalls some discussion we had about this but forgets the conclusion)

Comment on lines +166 to +197
eprintln!();
eprintln!("stdout:");
stderr().write_all(&result.stdout).into_diagnostic()?;
Copy link
Member

Choose a reason for hiding this comment

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

this is a weird juxtaposition... but ok i think i see why

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This is for consistency with the cargo builds, where we have things which consume the underlying build's stdout. I'd run into cases where allowing the output to go to stdout caused other parts of our build process to fail. (This code is actually from the original generic builds code, just duplicated here.)

build: extra.build.to_owned(),
artifact: target_path.to_owned(),
}),
checksum: None,
Copy link
Member

Choose a reason for hiding this comment

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

hmm i imagine we'll add a flag for that one day

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I figured we could talk about what we wanted here and maybe add it later.

cargo-dist/src/tasks.rs Outdated Show resolved Hide resolved
cargo-dist/src/main.rs Outdated Show resolved Hide resolved
cargo-dist/src/tasks.rs Outdated Show resolved Hide resolved
@mistydemeo mistydemeo force-pushed the extra-artifacts branch 3 times, most recently from cfc695a to a267a0f Compare November 28, 2023 21:55
This allows building and packaging arbitrary extra artifacts alongside code
tarballs. For example, cargo-dist itself uses it for its own schema metadata JSON.
@mistydemeo mistydemeo force-pushed the extra-artifacts branch 3 times, most recently from cf61ea8 to 60f5d13 Compare November 29, 2023 17:30
@mistydemeo mistydemeo merged commit 28c2bd1 into main Nov 29, 2023
14 checks passed
@mistydemeo mistydemeo deleted the extra-artifacts branch November 29, 2023 18:05
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.

Additional non-Rust artifacts in the same Github release
2 participants