rust-lang / rustc_codegen_cranelift

Cranelift based backend for rustc
Apache License 2.0
1.52k stars 94 forks source link

Implement vendoring #1342

Closed bjorn3 closed 1 year ago

bjorn3 commented 1 year ago

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 commented 1 year ago

@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 commented 1 year ago

./y.rs vendor will need to run after https://github.com/rust-lang/rust/blob/af669c26846f85fd15e34a6f03d5d2f237444c17/src/bootstrap/dist.rs#L995

jyn514 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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 commented 1 year ago

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.