Skip to content

Add the Chrome OS executor #6106

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 1 commit into from

Conversation

jynnantonix
Copy link

I don't expect this pull request to be merged. It makes several assumptions that are only valid in the chrome os build environment and it simply panics if it runs into any errors. Instead I'm sending this pull request as a desperate cry for help and as a sign of how bad things have gotten for at least one team (Chrome OS) that is trying to actually ship rust code as a part of a larger system.

Chrome OS builds hundreds of different software packages as part of the OS image. Rebuilding the same dependency crate 17 times for 17 different rust packages is simply not going to work for us. #1139 has been open for more than 3 years and RFC 2136 was merged more than 7 months ago but as far as I can tell no actual progress has been made towards better build system integration.

Now let me describe how this patch is supposed to work.

The Chrome OS executor is designed to work with a local, directory-based registry. We tell cargo to use this registry instead of crates.io via its .cargo/config. Crates with no dependencies (for example, the libc crate) are built as normal. When publishing a crate to the registry we install the crate's Cargo.toml, the built rlib for the crate, and an empty src/lib.rs file. Later when cargo tries to rebuild a crate from the local registry (as a dependency of some other crate), the chrome os executor will detect this and use the pre-built rlib instead of calling rustc. The empty src/lib.rs file is there as a backup: if we somehow do end up building the crate from source, rustc will produce an empty crate and will cause a compile error if anything tries to use that crate.

There are some additional tweaks on top of that basic usage.

Chrome OS ships both arm and x86 devices so we need to be able to cross-compile everything. Since cargo expects full sources in the registry, there is no way to detect ahead of time whether a crate is going to be used on the target machine or the build machine (maybe as part of a build.rs step). To deal with this, we build each crate twice: once for the target architecture and once for the build machine architecture. Both rlibs are installed in the registry in the appropriate directory. The chrome os executor uses the --target flag that cargo passes to rustc to determine which rlib to use and then provides the correct one.

Cargo requires every possible dependency of the crate to exist in the registry even if they will never actually be used during the compile step. This includes things like the winapi crate even when building for linux. To deal with this we install empty crates in the local registry. These crates have a minimal Cargo.toml (just the name and version with no dependencies) and an empty src/lib.rs file. These crates have no pre-built rlibs so if they are ever used at compile time the chrome os executor will panic because it cannot find an rlib for them.

Dealing with new toolchain releases is relatively straightforward. Chrome OS uses portage as its build system. All rust packages declare a special type of dependency on the rust toolchain package so that if the toolchain version ever changes, all rust packages are rebuilt from source. This ensures that we don't use rlibs from different rustc versions in the same compile.

I'm sure that I've probably made some terrible assumptions but the fact is that cargo in its current form simply cannot provide what we need in terms of build system hooks. I'm willing to implement better integration for cargo that works for more than just chrome os. Cargo is large and complex and this is the best starting point I could find.

Please please please prioritize better build system integration for cargo.

The Chrome OS executor is designed to work with the Chrome OS build
system.  It expects pre-built rlibs for both the host and target
machines to exist in the crate registry.

When the executor detects that cargo is attempting to compile an rlib it
uses the rlib from the registry instead of invoking rustc.

Signed-off-by: Chirantan Ekbote <[email protected]>
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @matklad (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nrc
Copy link
Member

nrc commented Sep 28, 2018

Hi, thanks for the PR and for explaining your use case in such detail! Unfortunately, Cargo is not in a great place for integration with other build systems (as you've discovered). You've also touched on a few things (such as cached builds) that lots of users want for Cargo. The general case for these features has been a lot of work to design since it is really taking Cargo in a new direction from top to bottom. We're already starting some experiments towards interacting with other build systems, but a big effort is not possible until after the 2018 edition release. However, these things are a very high priority for us. It's great that you're able to help out - we'd love the help! I'll get back in touch once we start the design work.

As you note, this PR probably can't land as is. However, the Executor API is designed to be used by tools outside Cargo (the RLS is the only example I'm aware of). So I think by creating a thin wrapper around Cargo and using the ChromeOsExecutor you should be able to get all the benefit of this PR. If you need additional small pieces of Cargo to be made public for that, then we'd be happy to accept that kind of PR.

@nrc nrc closed this Sep 28, 2018
@jynnantonix
Copy link
Author

Thanks for the quick reply. We are currently patching cargo locally with this patch so things work in our system.

We're more than happy to test any new build system integration changes so please keep us informed.

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