Skip to content

Implement vendoring #1342

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

Closed
wants to merge 2 commits into from
Closed

Implement vendoring #1342

wants to merge 2 commits into from

Conversation

bjorn3
Copy link
Member

@bjorn3 bjorn3 commented Jan 16, 2023

Part of https://github.com/bjorn3/rustc_codegen_cranelift/issues/1290

./y.rs vendor will vendor everything that needs to be fetched from the internet into download/. After vendoring you shouldn't use ./y.rs prepare.

@bjorn3 bjorn3 force-pushed the support_vendoring branch from d6677c4 to 248a67a Compare January 16, 2023 17:38
@bjorn3
Copy link
Member Author

bjorn3 commented Jan 16, 2023

@jyn514 will this work for integration with rust's CI? The vendored directory will be separate from the main one. It is 395MB compared to rust's 602MB. Gzip compressed it is 54MB compared to rust's 81MB. Vendoring just cg_clif takes 208MB or 135MB when removing the jit feature. The total package is 355MB when removing the jit feature. Part of this size difference between jit and no jit comes from region and libloading still depending on winapi rather than windows-sys.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 16, 2023

@jyn514
Copy link
Member

jyn514 commented Jan 17, 2023

Hmm, I'm confused how vendoring is related to x.py test? Making it possible to build cg_clif from a source tarball seems useful, but I'm not sure why it would be necessary for gettting tests working.

The vendored directory will be separate from the main one.

What's the reason for that? Will it shrink the size at all if we combine them?

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 17, 2023

Hmm, I'm confused how vendoring is related to x.py test? Making it possible to build cg_clif from a source tarball seems useful, but I'm not sure why it would be necessary for gettting tests working.

My plan was to enable the testing by default, which means it would also run for source tarballs. If you think I should rather only run it on rust's CI, but not by default when using ./x.py test, I guess that is fine too. It may just confuse people when CI fails despite it working locally.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 17, 2023

What's the reason for that? Will it shrink the size at all if we combine them?

Because a second cargo vendor call seems to remove crates vendored by the previous call, thus requiring a single cargo vendor call for both the main rust workspace and cg_clif. Merging both should save at least 208MB, although that can likely also be saved by removing the vendor call of cg_clif itself from x.py and instead using the version vendored here when building the cranelift backend from x.py as opposed to cg_clif's y.rs.

@jyn514
Copy link
Member

jyn514 commented Jan 19, 2023

My plan was to enable the testing by default, which means it would also run for source tarballs. If you think I should rather only run it on rust's CI, but not by default when using ./x.py test, I guess that is fine too. It may just confuse people when CI fails despite it working locally.

Got it - that seems like a good goal 👍

Because a second cargo vendor call seems to remove crates vendored by the previous call, thus requiring a single cargo vendor call for both the main rust workspace and cg_clif. Merging both should save at least 208MB, although that can likely also be saved by removing the vendor call of cg_clif itself from x.py and instead using the version vendored here when building the cranelift backend from x.py as opposed to cg_clif's y.rs.

Ahhh right, I've run into that bug too :(

It sounds like cg_clif itself doesn't need vendoring and it's just for the benefit of the rust source tarballs, right? In that case, could we move the vendoring to x.py, around https://github.com/rust-lang/rust/blob/af669c26846f85fd15e34a6f03d5d2f237444c17/src/bootstrap/dist.rs#L976-L985 ? That should combine both into a single vendor/ directory, hopefully without breaking anything for your repository.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 19, 2023

It sounds like cg_clif itself doesn't need vendoring and it's just for the benefit of the rust source tarballs, right?

Indeed

In that case, could we move the vendoring to x.py, around https://github.com/rust-lang/rust/blob/af669c26846f85fd15e34a6f03d5d2f237444c17/src/bootstrap/dist.rs#L976-L985 ? That should combine both into a single vendor/ directory, hopefully without breaking anything for your repository.

The part of ./y.rs vendor which is also performed by ./y.rs prepare (downloading several git repos) is not something I did like to see duplicated as it would be way too easy for the copies to get out of sync. The cargo vendor invocation could be moved there I guess by first running ./y.rs prepare and then searching the produced downloads/ dir for all directories with a Cargo.toml file to determine what workspaces need to be vendored. (Combined with downloads/sysroot/sysroot_src/library/core/tests as that one is a separate workspace due to me not being able to figure out how to make cargo test -p core work.)

@jyn514
Copy link
Member

jyn514 commented Jan 19, 2023

The cargo vendor invocation could be moved there I guess by first running ./y.rs prepare and then searching the produced downloads/ dir for all directories with a Cargo.toml file to determine what workspaces need to be vendored.

If that's not too much extra work, I think that would be my preference, seems nice to save the space since we store release artifacts indefinitely. That said, we can always fix it later, I don't mind merging this for now if the new code is a hassle.

@bjorn3
Copy link
Member Author

bjorn3 commented Jan 19, 2023

I think it wouldn't be too much of a hassle. I will cherry-pick the TOCTOU commit directly to the master branch (there were a couple of other occurences) and close this PR.

@bjorn3 bjorn3 force-pushed the support_vendoring branch from c3fa16e to e1e70e9 Compare January 19, 2023 16:01
@bjorn3 bjorn3 closed this Jan 19, 2023
@bjorn3 bjorn3 deleted the support_vendoring branch January 19, 2023 16:01
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.

2 participants