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(dist): add cargo-dist #559

Merged
merged 2 commits into from
May 22, 2024
Merged

feat(dist): add cargo-dist #559

merged 2 commits into from
May 22, 2024

Conversation

ashleygwilliams
Copy link
Contributor

@ashleygwilliams ashleygwilliams commented Nov 15, 2023

addresses #552 - marked as draft as i made some calls about what targets and installers you'd like to support. happy to talk through those and update.

you can run cargo dist plan locally to see what this workflow will generate. i've also put the output here:

announcing v0.4.0
  cargo-cyclonedx 0.4.0
    cargo-cyclonedx-installer.sh
    cargo-cyclonedx-installer.ps1
    cargo-cyclonedx-aarch64-apple-darwin.tar.xz
      [bin] cargo-cyclonedx
      [misc] CHANGELOG.md, LICENSE, README.md
      [checksum] cargo-cyclonedx-aarch64-apple-darwin.tar.xz.sha256
    cargo-cyclonedx-x86_64-apple-darwin.tar.xz
      [bin] cargo-cyclonedx
      [misc] CHANGELOG.md, LICENSE, README.md
      [checksum] cargo-cyclonedx-x86_64-apple-darwin.tar.xz.sha256
    cargo-cyclonedx-x86_64-pc-windows-msvc.zip
      [bin] cargo-cyclonedx.exe
      [misc] CHANGELOG.md, LICENSE, README.md
      [checksum] cargo-cyclonedx-x86_64-pc-windows-msvc.zip.sha256
    cargo-cyclonedx-x86_64-unknown-linux-gnu.tar.xz
      [bin] cargo-cyclonedx
      [misc] CHANGELOG.md, LICENSE, README.md
      [checksum] cargo-cyclonedx-x86_64-unknown-linux-gnu.tar.xz.sha256
    cargo-cyclonedx-x86_64-unknown-linux-musl.tar.xz
      [bin] cargo-cyclonedx
      [misc] CHANGELOG.md, LICENSE, README.md
      [checksum] cargo-cyclonedx-x86_64-unknown-linux-musl.tar.xz.sha256

@ashleygwilliams
Copy link
Contributor Author

to test this out we could push a pre-release tag and it would generate a github release. let me know if that sounds like a good plan and if you'd like to add or remove any targets or installers i picked.

@Shnatsel
Copy link
Contributor

Thanks for the PR! I'll take a closer look tomorrow.

@Shnatsel Shnatsel self-assigned this Nov 15, 2023
@Shnatsel
Copy link
Contributor

Thanks! Two things of note:

  1. We would need to also remove the build steps added in #488
  2. Is there an easy way to add aarch64 Linux as well? I'm pretty sure cross can handle that.

@ashleygwilliams
Copy link
Contributor Author

i can write up an additional workflow that could do aarch64 linux- cargo-dist doesn't currently support it (the workflow committed in this PR is fully generated, i can also hand edit the release workflow, but it's generally better to add an additional workflow that does the additional target). and happy to remove the other workflow.

@Shnatsel
Copy link
Contributor

In that case I'd prefer to stick to the fully generated workflow in the interest of reducing the maintenance burden and making upgrading to newer cargo dist easier. I don't want aarch64 linux prebuilt binaries badly enough to hand-roll them.

The previous release workflow would need to be removed, but other than that this looks good to go.

@ashleygwilliams
Copy link
Contributor Author

ok cool! also, now that we have a request for aarch64 linux we can look into getting it into cargo-dist :) i'll get the changes up momentarily for this PR

@Shnatsel
Copy link
Contributor

Thanks!

My thinking is that Raspberry Pis need prebuilt binaries really badly, and ARM servers are increasingly popular as well because of the way cloud providers price them.

But it's not really needed for cargo cyclonedx. You would usually run cargo cyclonedx either on the developer machine or on CI, but RPis make terrible developer machines, and there is still no CI provider with easily available (let alone free) ARM servers.

@lfrancke
Copy link
Contributor

That said... We just build an ARM GitHub runner and would love this :) but yeah... Not too important

@ashleygwilliams
Copy link
Contributor Author

hey! realized i fell off on this and would love to pick it back up. will update to the latest cargo-dist now, and let me know what else i need to add to get this over the finish line! sorry about the delay

@ashleygwilliams ashleygwilliams force-pushed the cargo-dist branch 2 times, most recently from 2f69762 to ace0a2d Compare March 18, 2024 18:29
@ashleygwilliams
Copy link
Contributor Author

ashleygwilliams commented Mar 18, 2024

re linux arm- if there's acgustom runner we can point to i can add this as a target. at axo we use buildjet for this- costs us an incredibly small amount of money, would recommend (but also understand not wanting to).

@Shnatsel
Copy link
Contributor

The PR is still marked as draft. I understand it is ready for review now?

@ashleygwilliams ashleygwilliams marked this pull request as ready for review March 19, 2024 14:05
@ashleygwilliams ashleygwilliams requested a review from a team as a code owner March 19, 2024 14:05
@lfrancke
Copy link
Contributor

lfrancke commented Apr 4, 2024

@Shnatsel this is ready for review now. Would you be willing to take a look?

@Shnatsel
Copy link
Contributor

Shnatsel commented Apr 4, 2024

Yes! Thank you. I will take a look in the next few days.

@stevespringett
Copy link
Member

Any chance of getting a review of this PR?

@Shnatsel
Copy link
Contributor

Sorry for dragging my feet on the review.

Unfortunately this runs into some thorny issues with the way the crates are currently published. None of the active maintainers have access to the publishing token, and the original maintainers are incommunicado. This leaves us with publishing releases to crates.io using a token stored in a github action secret.

We recently had to add another crate to the project, because procedural macros require a separate crate for technical reasons. This complicates the release process even further. I want to sort out the release process with the extra added crate first, which is required for the crucial CycloneDX v1.5 support in the library, and only then deal with further less critical changes including this PR.

@stevespringett
Copy link
Member

@Shnatsel ping me on Slack and we'll sort it out.

@Shnatsel
Copy link
Contributor

There already is a workflow we inherited from the previous maintainers that compiles and publishing binaries to Github releases. Having two of them at the same time is obviously not a good idea. Compiling binaries should be disentangled from the publishing workflow before I can land this PR.

I am being extra cautious with changes to the publishing workflow because without access to the publishing token we wouldn't be able to yank published crates, which makes correcting any mistakes in the release process very difficult.

@ashleygwilliams
Copy link
Contributor Author

ashleygwilliams commented Apr 29, 2024

a point that may be valuable to note: this current workflow does not include publishing to crates.io. i agree that extra consideration should be made there because it is a one way door. the pr included is for building binaries, and then also building a curl | sh installer, and a powershell installer. these installers are preferable on CI platforms because they do not require compilation (which a cargo install does). we can also add a homebrew and npm installer if that's desirable with a bit of config, but that is not part of this PR.

everything in this PR generates something that is a two way door: all bins, installers, and github releases are mutable (deletable/fixable).

@ashleygwilliams
Copy link
Contributor Author

it is possible that a sync conversation about this may help any confusion. i am happy to make myself available on slack and/or a zoom call

@Shnatsel
Copy link
Contributor

I understand that this PR does not publish to crates.io. We have a legacy workflow which does, and which also compiled binaries. The binary publishing part will have to be removed from the legacy workflow before this one can be merged. This should (hopefully) be as easy as reverting #488. Actually doing that is a prerequisite for landing this PR.

We already have a Github release with published binaries for the current version of cargo cyclonedx, so this PR will only take effect once we cut a new release. So here is my proposal for an actionable plan to get this merged:

  1. Complete CycloneDX v1.5 support in cyclonedx-bom crate, tracked as Support spec version 1.5 #646 (@justahero)
  2. Create a publishing workflow for cyclonedx-bom-macros crate and publish an initial release (@pvdrz, @Shnatsel)
  3. Publish a new release of cyclonedx-bom crate (@Shnatsel)
  4. Update cargo cyclonedx with spec version 1.5 support (@Shnatsel)
  5. Revert Add prod build and release steps #488 and double-check for any other edits required to the legacy publishing/tagging workflow (@Shnatsel)
  6. Double-check the tag names in this PR and make sure everything aligns with the legacy publishing/tagging workflow, then merge this PR (@Shnatsel)
  7. Publish a new version of cargo cyclonedx, finally making it accessible to cargo binstall et al.

My guesstimate for the ETA on all of this is 2 weeks.

I hope this sounds good?

@Shnatsel
Copy link
Contributor

Shnatsel commented May 6, 2024

We have sorted out the publishing permissions (thanks, Steve!) so the release process should be a lot less risky now.

I was about to ask how to go about upgrading to a newer version, but the cargo-dist book already covers it. Nice!

@ashleygwilliams
Copy link
Contributor Author

ashleygwilliams commented May 7, 2024

@Shnatsel awesome news. let me know how i can help. additionally- if it comes up that there's targets or package manager systems this project needs that cargo-dist does not yet support, we are extremely open and excited about feature requests, so don't hesitate to let us know.

once this is merged, my goal is to integrate cyclonedx BOM generation as a configurable feature in cargo-dist. i've filed an issue to track work on it here: axodotdev/cargo-dist#1016

Signed-off-by: Ashley Williams <ashley666ashley@gmail.com>
Signed-off-by: Ashley Williams <ashley666ashley@gmail.com>
@Shnatsel
Copy link
Contributor

I've published cyclonedx-bom to crates.io and opened #714 to delete our custom publishing workflow in preparation for cargo dist.

@Shnatsel
Copy link
Contributor

The CI failure is due to an issue with the nightly compiler, already worked around in main. So I'm going to go ahead and merge this PR.

All right, let's give this a shot!

@Shnatsel Shnatsel merged commit 6ae06ec into CycloneDX:main May 22, 2024
12 of 14 checks passed
@Shnatsel
Copy link
Contributor

Well, the tag for v0.5.1 has been created, but I don't think the release workflow has been triggered. Any idea why?

@ashleygwilliams
Copy link
Contributor Author

hrmmmm, let me see

@ashleygwilliams
Copy link
Contributor Author

ok yes! the reason is the tag structure - looking into how we can fix this, one sec

@ashleygwilliams
Copy link
Contributor Author

actually that's not correct at all. i am not sure what's going on, but i have to assume it's CI related- as everything works correctly locally, and we fully support the tag structure you have. continuing to investigate.

@Shnatsel
Copy link
Contributor

The tag structure looks correct to me. This is the code that pushes the tag:

run: |
git config --local user.email "41898282+github-actions[bot]@users.noreply.github.com"
git config --local user.name "github-actions[bot]"
git commit -am "New development bump of cargo-cylonedx to ${{steps.version.outputs.CARGO_VERSION}}"
git tag -a "cargo-cyclonedx-${{steps.version.outputs.CARGO_VERSION}}" -m "cargo-cyclonedx ${{steps.version.outputs.CARGO_VERSION}} release"
git push --follow-tags

Also nothing out of the ordinary there, it is a push. Perhaps Github has some protections against actions triggering other actions, so that you don't end up in an infinite loop?

@ashleygwilliams
Copy link
Contributor Author

so i have been running some tests on our fork here: https://github.com/axodotdev/cyclonedx-rust-cargo/actions/runs/9197176872

i think that you are right- there is some sort of protection if the tag is originating from an action itself it seems?

@ashleygwilliams
Copy link
Contributor Author

ah @mistydemeo found this https://github.com/orgs/community/discussions/27028#discussioncomment-3254360 which seems to suggest that it's not possible to trigger one action from another

@Shnatsel
Copy link
Contributor

Okay, I see that I'll need to gut the release workflow and remove tagging from it.

In the meantime, how do I trigger the cargo-dist workflow for v0.5.1? Is deleting and recreating the tag sufficient?

@ashleygwilliams
Copy link
Contributor Author

apologies for the delay, yes deleting and recreating the tag will be sufficient to trigger the flow. so sorry for this oversight and the hassle that ensued! i have learned of some (arguably not pleasant) workarounds if you'd like to try to keep the tagging in the workflow. likely best to do manually for now but then we can discuss

@Shnatsel
Copy link
Contributor

I've manually recreated the tag, and the binary release is now up: https://github.com/CycloneDX/cyclonedx-rust-cargo/releases/tag/cargo-cyclonedx-0.5.1

However, I am seeing these warnings printed multiple times in the global artifact workflow:
WARN !!! duplicate system keys, platforms may get conflated !!!
see https://github.com/CycloneDX/cyclonedx-rust-cargo/actions/runs/9213590201/job/25348014566

I admit I do not understand what "system keys" are. Is this a misconfiguration on our side?

Also, the Markdown extracted from the changelog that goes onto the release page doesn't handle links correctly when the URL is provided separately instead of inline. Note the release page showing ([#559]) while the full rendered changelog shows (#559). This is not a big deal for us since we're just linking to PRs anyway, but it figured I'd let you know.

@ashleygwilliams
Copy link
Contributor Author

thanks for letting me know about the markdown thing. we are not formally parsing the markdown so i wonder if this is a github thing. regardless i'll take a further look. i know that we have users who do similar style linking, but we haven't seen this so i'm curious what's going on.

as for that warning... i have never seen that in the wild. only development, and very early on. i'm running a test right now (https://github.com/axodotdev/cyclonedx-rust-cargo/actions/runs/9213869039). i am wondering if it is related to the bumpy tagging situation. once i can repro i'll dig in further. your release appears completely fine and the installers work great so i'm hoping it's a false positive. here's where in our code it is triggered: https://github.com/axodotdev/cargo-dist/blob/632d7584d0732c2ee10e19561f168811c4f92c38/cargo-dist/src/manifest.rs#L132

@ashleygwilliams
Copy link
Contributor Author

ok, confirmed. that warning is a false positive, you can safely ignore and we'll remove it for the next release

@Shnatsel
Copy link
Contributor

Thanks for looking into the warning!

i know that we have users who do similar style linking, but we haven't seen this so i'm curious what's going on.

I think your code just grabs everything under the appropriate header. Our changelog has the URLs for the links at the end of the file, so the URLs do not get included into the snippet posted to the release page.

But this is an edge case, and I'm not sure it's worth the trouble of introducing a markdown parser just to get such style of links to work. Especially since Markdown is so loosely specified and all the parsers end up being subtly incompatible anyway.

@Shnatsel
Copy link
Contributor

Now that the release binaries are published, it should be easy to integrate cargo cyclonedx into cargo dist. Please let me know if you have any questions or encounter any issues. I am happy to help!

Speaking of SBOMs, there are people requesting cargo auditable support in cargo dist too: axodotdev/cargo-dist#81
I assume having published binaries would simplify integration into cargo dist, so I'm going to go ahead and set that up as well.

@ashleygwilliams
Copy link
Contributor Author

oh that's super awesome. yes cargo auditable integration would definitely be welcome! will let you know. i dont know if you use discord but we have one: https://discord.gg/hucxPFB6. otherwise i'm honestly happy to just keep using this PR as a convo space :)

@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 4, 2024

@ashleygwilliams I've published a new release, v0.5.2, and cargo binstall no longer picks up on binaries from our GH releases made with cargo dist:

$ cargo binstall cargo-cyclonedx
 WARN Failed to retrieve token from `gh auth token` err=Os { code: 2, kind: NotFound, message: "No such file or directory" }
 WARN Failed to read git credential file
 INFO resolve: Resolving package: 'cargo-cyclonedx'
 WARN The package cargo-cyclonedx v0.5.2 will be installed from source (with cargo)
Do you wish to continue? yes/[no]
? 

If I use --version=0.5.1 it downloads a binary from a third-party build service, so we get a binary but it isn't sourced from our Github releases:

cargo binstall --version=0.5.1 cargo-cyclonedx
 WARN Failed to retrieve token from `gh auth token` err=Os { code: 2, kind: NotFound, message: "No such file or directory" }
 WARN Failed to read git credential file
 INFO resolve: Resolving package: 'cargo-cyclonedx'
 INFO resolve: Verified signature for package 'cargo-cyclonedx-0.5.1-x86_64-unknown-linux-gnu': timestamp:1716442392	file:cargo-cyclonedx-0.5.1-x86_64-unknown-linux-gnu.tar.gz	hashed
 WARN The package cargo-cyclonedx v0.5.1 (x86_64-unknown-linux-gnu) has been downloaded from third-party source QuickInstall
 INFO This will install the following binaries:
 INFO   - cargo-cyclonedx (cargo-cyclonedx -> /home/shnatsel/.cargo/bin/cargo-cyclonedx)
Do you wish to continue? yes/[no]
? 

Which leads me to believe that cargo dist never really worked with cargo binstall on our repo after all.

Could you take a look and see what's up with that?

@ashleygwilliams
Copy link
Contributor Author

@Shnatsel oh huh! that is very weird, and yeah we'll take a look right now. cargo binstall works on cargo-dist and so i wonder if there's something perhaps with tag structure or the workspace that is making the lookup fail

@ashleygwilliams
Copy link
Contributor Author

ashleygwilliams commented Jun 4, 2024

ok! so the issue is that your tag structure is not one of the two default tag structures that binstall looks up. if you run cargo binstall cargo-cyclonedx --dry-run --verbose you can see this:

DEBUG Checking for package at: 'https://github.com/CycloneDX/cyclonedx-rust-cargo/releases/download/0.5.2/cargo-cyclonedx-x86_64-pc-windows-msvc.zip'

DEBUG Checking for package at: 'https://github.com/CycloneDX/cyclonedx-rust-cargo/releases/download/v0.5.2/cargo-cyclonedx-x86_64-pc-windows-msvc.zip'

luckily, binstall lets you write config (docs) to change the structure of the tag and the package url in general. you'll probably want something like this:

pkg-url = "{ repo }/releases/download/cargo-cyclonedx-{ version }/{ name }-{ target }{ archive-suffix }"

you can make sure this is 100% correct by running the dry run command from above.

because binstall uses crates.io metadata as their source of truth, this config won't apply retroactively to previous version but all future versions with this config should work.

@ashleygwilliams
Copy link
Contributor Author

(sorry for not catching this sooner, we should honestly doc this better on our end and will do so!)

@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 4, 2024

Yeah, it would be neat if cargo dist would print a warning when the tag structure doesn't match what cargo binstall expects, and point me to the cargo-binstall documentation.

The config snippet you provided does indeed work. I've opened a PR to add it: #727

Thanks a lot!

@ashleygwilliams
Copy link
Contributor Author

@Shnatsel yeah that's an interesting idea. my thought was honestly wondering if cargo dist init could write the binstall config for you. this would require knowing your tag structure, so i'll have to think through how we might want to do that, but i agree there's some work here to make this a nicer experience!!

@Shnatsel
Copy link
Contributor

Shnatsel commented Jun 4, 2024

It could also verify that it matches in CI on pull requests. There's already tag verification at that stage.

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.

4 participants