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

Improve ci - install binaries instead of compiling #12643

Closed
wants to merge 2 commits into from

Conversation

ameknite
Copy link
Contributor

Objective

Reduce time compiling crates in CI.

Solution

install-actions uses github releases to find the binaries.
Uses cargo-binstall as a fallback

@ameknite ameknite changed the title Improve ci - install binaries instead of compiling with cargo install Improve ci - install binaries instead of compiling Mar 22, 2024
@matiqo15 matiqo15 added A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times labels Mar 22, 2024
@james7132 james7132 requested a review from mockersf March 23, 2024 01:26
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

Good change, I'm going to try to rally on Discord to get this merged!

@BD103
Copy link
Member

BD103 commented Apr 22, 2024

Also would you mind rebasing onto main? A few of the required checks have been modified after this PR was created, which is why it says some checks haven't been completed yet.

Copy link
Member

@mockersf mockersf left a comment

Choose a reason for hiding this comment

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

please put the version of the action everywhere

@BD103
Copy link
Member

BD103 commented Apr 22, 2024

please put the version of the action everywhere

Agreed, it would be good to target specific versions of the binaries so we don't have breaking changes.

- uses: taiki-e/install-action@v2
  with:
    tool: [email protected]

I don't think we need to specify the patch version, but certainly the minor and major versions.

@TimJentzsch TimJentzsch self-requested a review April 24, 2024 14:17
Copy link
Contributor

@TimJentzsch TimJentzsch left a comment

Choose a reason for hiding this comment

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

Do you have some measurements on how much this improves the CI times?

It seems like this adds two additional security risks:

  • The install-action action itself, which runs multiple shell scripts
  • The release binaries which are being downloaded (and might not match the source code)

Of course, there are always some risks involved when adding new dependencies and we can mitigate them by fixing the version of both the action (note that the commit hashes are safer than the version tags) and the installed tools.
Just wondering if it's worth it in this case

@BD103 BD103 added the X-Contentious There are nontrivial implications that should be thought through label May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Build-System Related to build systems or continuous integration C-Performance A change motivated by improving speed, memory usage or compile times X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants