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

Consider using cargo binstall for rust crates #2382

Closed
echoix opened this issue Feb 21, 2023 · 16 comments
Closed

Consider using cargo binstall for rust crates #2382

echoix opened this issue Feb 21, 2023 · 16 comments
Labels
O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity question Further information is requested

Comments

@echoix
Copy link
Collaborator

echoix commented Feb 21, 2023

In the weekend I played a lot to see how to reduce Rust crates installing time (first time playing around rust compilation).
Current times are 5-6 minutes for 1 linter (clippy is installed with rust in the first 90 secs) and 2 sarif tools used for other linters. Whereas our total build times are between 20-25 min. So 20-25% of the time is spent on these two tools.

I found that cargo binstall would help a lot https://github.com/cargo-bins/cargo-binstall. It is not preinstalled, but downloadable (precompiled -> it is optimized better than when using cargo install cargo-binstall as per their docs). cargo binstall downloads the binary from the github releases if available for the required target (the triplet). If not, it will use cargo-quickinstall, another project, and will fall back to cargo install, ie: compile from source. It seems supported by multiple projects as I saw, that's nice to see. The download URL format for the crates are specified in the project's Cargo.toml, and this file is the manifest that is used for crates.io. (theres an override possible too). sarif-fmt and shellcheck-sarif are already configured for binstall. However, they only include two binaries in their releases and they do not include our triplets, so they compile from source.

The current command that is used to install our two requirements (appart from clippy, that is a component)

cargo install --force --locked sarif-fmt shellcheck-sarif

I had one conceptual blocker beforehand:
What where the design choices that lead to adding the --force and --locked options? These didn't really work with binstall, and could figure out if they were really important.

If using in the dockerfile RUN step and compile from source using this command,

cargo binstall --no-confirm sarif-fmt shellcheck-sarif

is faster than the current command (docker step finishes in 120-130 sec instead of 320-380 sec on the github instance i was using (up to 16 cores could have been used). In github actions I think I already saw a 8 minutes for the rust step. I think it might be --force that makes recompiling some packages. Why is it needed?

For the binstall, I had a proof of concept that worked quite well.

##############################
# Installs rust dependencies #
#############################################################################################
## @generated by .automation/build.py using descriptor files, please do not update manually ##
#############################################################################################

#CARGO__START
RUN curl https://sh.rustup.rs -sSf | sh -s -- -y --profile minimal --default-toolchain stable \
    && export PATH="/root/.cargo/bin:${PATH}" \
    && wget --tries=5 -q -O cargo-binstall--unknown-linux-musl.tgz https://github.com/cargo-bins/cargo-binstall/releases/latest/download/cargo-binstall-x86_64-unknown-linux-musl.tgz \
    && tar xf cargo-binstall--unknown-linux-musl.tgz --directory /root/.cargo/bin \
    && cargo binstall --no-confirm cargo-bloat \
    && cargo binstall --no-confirm sarif-fmt  shellcheck-sarif \
    # && cargo install --force --locked sarif-fmt  shellcheck-sarif  \
    && rm -rf /root/.cargo/registry /root/.cargo/git /root/.cache/sccache /root/.rustup
ENV PATH="/root/.cargo/bin:${PATH}"
#CARGO__END

I didn't clean up the binstall binary yet.

At the very end of the time spent yesterday I started working on a workflow for the sarif-rs repo to build the tools as static binaries (ie: with musl with triplet x86_64-unknown-linux-musl instead of the dynamic shared libraries version using gcc, triplet x86_64-unknown-linux-gnu). I really didn't finish, I was just starting.

@echoix echoix added the question Further information is requested label Feb 21, 2023
@echoix
Copy link
Collaborator Author

echoix commented Feb 21, 2023

Depending on your answers, we might still benefit from setting up binstall right now even before the packages are available in a release.

@Kurt-von-Laven
Copy link
Collaborator

It all sounds reasonable to me, but I don't have any Rust experience.

@nvuillam
Copy link
Member

I also noticed than rust stuff takes soooooooo much time, if you make your solution work, I'll gladly merge the PR <3

@echoix
Copy link
Collaborator Author

echoix commented Feb 21, 2023

Even before my two Sunday PRs were merged I was on that :) in the beginning of that day I had a solution imagined for time taken for tests, I'm not sure yet if it's adequate yet.

@echoix
Copy link
Collaborator Author

echoix commented Feb 21, 2023

I also noticed than rust stuff takes soooooooo much time, if you make your solution work, I'll gladly merge the PR <3

My initial issue, before finishing writing it, was specifically to ask about the two flags used in cargo install, why are they there?

@echoix
Copy link
Collaborator Author

echoix commented Feb 21, 2023

It all sounds reasonable to me, but I don't have any Rust experience.

Me neither, but I started to learn a bit about it. That's why I like working on MegaLinter, it feeds my curiosity 😄

@bdovaz
Copy link
Collaborator

bdovaz commented Feb 21, 2023

@echoix same here, two months ago I didn't even know how to do a Hello World in Python 🤣.

@nvuillam
Copy link
Member

I learned python especially to build MegaLinter, and I never heard about a quarter of everything that we can lint 🤣

@Kurt-von-Laven
Copy link
Collaborator

I was inspired to read the cargo install docs, and --force strikes me as the sort of thing you're only supposed to use as a fallback if you encounter problems without it. I think you're right that it has a significant build time cost. Ideally we would configure Cargo to pass at least --locked through to cargo install when cargo binstall falls all the way back to cargo install, but I wouldn't worry about it for this iteration if there's no straightforward way to achieve such a configuration. May this thread be inspiration to newbies everywhere!

@echoix
Copy link
Collaborator Author

echoix commented Feb 22, 2023

I was inspired to read the cargo install docs, and --force strikes me as the sort of thing you're only supposed to use as a fallback if you encounter problems without it. I think you're right that it has a significant build time cost. Ideally we would configure Cargo to pass at least --locked through to cargo install when cargo binstall falls all the way back to cargo install, but I wouldn't worry about it for this iteration if there's no straightforward way to achieve such a configuration. May this thread be inspiration to newbies everywhere!

At that point I'd rather the build to fail in order to look at it, if three installation methods, including the one that should work, fails.

@Kurt-von-Laven
Copy link
Collaborator

That makes sense to me for --force. I don't think the purpose of --locked is to prevent build failure though, merely to enforce reproducibility?

@echoix
Copy link
Collaborator Author

echoix commented Feb 22, 2023

I would have to retry, but I'm not sure binstall accepted the --locked flag. It doesn't make sense to have reproducibility on a already compiled binary, but it would if it needs to compile it from source

@Kurt-von-Laven
Copy link
Collaborator

To clarify, I was imagining that Cargo might have a config file where one can specify the equivalent setting.

@echoix
Copy link
Collaborator Author

echoix commented Feb 22, 2023

Oh, what a suprise, 11 hours ago a PR was merged to the cargo-binstall repo, especially about passing the --locked flag to the cargo install command if used: cargo-bins/cargo-binstall#830

@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 25, 2023
@Kurt-von-Laven Kurt-von-Laven removed the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Mar 28, 2023
@github-actions
Copy link
Contributor

This issue has been automatically marked as stale because it has not had recent activity.
It will be closed in 14 days if no further activity occurs.
Thank you for your contributions.

If you think this issue should stay open, please remove the O: stale 🤖 label or comment on the issue.

@github-actions github-actions bot added the O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity label Apr 28, 2023
@github-actions github-actions bot closed this as not planned Won't fix, can't repro, duplicate, stale May 13, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O: stale 🤖 This issue or pull request is stale, it will be closed if there is no activity question Further information is requested
Projects
None yet
Development

No branches or pull requests

4 participants