rust-lang / cargo

The Rust package manager
https://doc.rust-lang.org/cargo
Apache License 2.0
12.66k stars 2.41k forks source link

Should build-plans be deleted? #7614

Open alexcrichton opened 4 years ago

alexcrichton commented 4 years ago

The cargo team has been talking about this for some time now, and this is an issue I'd like to open to cc others and get some discussion about this.

The build-plan feature of Cargo is broken, and has always been broken. It has never actually accounted for all the build intricacies of crate graphs that Cargo supports. It was landed to unblock initial work with the understanding that it wouldn't be stabilized before it was fleshed out more, but the work to flesh it out more hasn't happened since then. The build plan does not support, for example:

There is not a known great solution for these fundamental issues, and there has not been much effort to investigate this. Cargo has received very little maintenance of build plans and new features in Cargo typically don't consider keeping build plans working, leading to issues like https://github.com/rust-lang/cargo/pull/7604. Additionally build plans are only very lightly tested, so there's not a huge amount of protection against regressions.

Given all this the Cargo team is leaning towards deleting this feature. The existence of the feature seems to give the guise that it's supported and officially recommended, when in fact it's neither supported nor recommended. The feature, as implemented, seems dead in the water without significant refactorings that may require starting from scratch anyway.

The feature, however, currently has what we believe is enough users that we don't want to just outright delete it. We'd like to cc some folks who we believe are using this feature on this issue. If you're not using the feature or you're not interested, sorry for the cc but feel free to unsubscribe yourself!

The Cargo team is specifically interested in asking others if they are aware of the downsides of the build plan feature, and also if others are available to help fix this feature and implement a working system. The Cargo team does not have the bandwidth to either design a working build plan or implement it at this time, so both the design and implementation work is what we're interested in getting help for.

While it's useful to list out the bugs in the current build plan, the Cargo team is hesitant to land fixes such as https://github.com/rust-lang/cargo/pull/7604 at this time. These "small fixes" perpetrate the sub-par integration of build plans into Cargo today and makes the codebase harder to maintain. What we're particularly interested is figuring out a way to get build plans onto more solid footing such that they can actually be supported.

cc @edwin0cheng cc @vlad20012 cc @matklad cc @Manishearth

CAD97 commented 4 years ago

FWIW, IntelliJ IDEA and rust-analyzer are only (atm) using build-plan to get the value of OUT_DIR. If there were some other way to get at it (e.g. #7546), then they wouldn't have any reason to use it anymore.

Manishearth commented 4 years ago

I'm not the right person to cc for this, you should ping someone who works on Firefox

vlad20012 commented 4 years ago

In IntelliJ Rust we use build-plan only to locate buildscript output directory (env!("OUT_DIR") in build.rs) for each crate in the graph. The ability to locate these directories is critical (but not really enough) to support code generation in the IDE. So, I'd be happy with a replacement like #7546.

(but in long-term, we need something like #7178 with the ability to get all outputs of all buildscripts in a dependency graph)

matklad commented 4 years ago

cc @Xanewok , I think build plans play a role in the non-cargo story for RLS.

I’ll write a bigger response once I am a the real keyboard, but in-short, my opinion is:

matklad commented 4 years ago

The core problem is that IntelliJ Rust needs a way to get OUT_DIR. They already implemented all the machinery for processing include!(concat!(env!("OUT_DIR"), "bindings.rs")). And handling such includes is important for user-experience of those who use code-generation.

It would be great to have some replacement feature for sniffing OUT_DIR. It doesn't have to be stable: tools have to track nightly for various reasons anyway.

Ideally, this info should be a part of cargo metadata, because IDEs really love the static picture of dependencies between the crates. Unfortunately, this can't work, because build scripts are fundamentally associated with units, and not with packages. That is, cargo build and cargo build --release will use (I think) different OUT_DIRs.

Long-term, we probably should do something like #7178, and maybe even try to move to "build.rs per package", and not "build.rs per unit" model.

For the time being, I like @ehuss suggestion: https://github.com/rust-lang/cargo/issues/7546#issuecomment-551948375. Basically, we just include OUT_DIR to the build-script json message, as if it printed cargo:rustc-env:OUT_DIR=/foo/bar. This seems like something we should just do. Should I send a PR ? :) This is forward compatible (and even a required part of) #7178. IntelliJ can then have a special UI action like "scrape build config", which is explicitly invoked by the user, runs cargo build --message-format=json and fishes out OUT_DIR (and cfg as well). That's a pretty awkward UX, but it is much better than nothing at all.

Just to give a better understanding of IDE requirements here, I believe the following small data-structure captures the information a Rust IDE needs pretty well:

https://github.com/rust-analyzer/rust-analyzer/blob/f4b1fb1554b639374adeffa50d4719f834a0d475/crates/ra_db/src/input.rs#L67-L110

It is much simpler than Cargo's internal representation, and the question is, how we lower Cargo's repr to this CrateGraph.

EDIT: PR to add OUT_DIR to --message-format=json: https://github.com/rust-lang/cargo/pull/7622

matklad commented 4 years ago

Note also that IDE use-case seems to be over-represented in this issue, while the original and main motivation for build plan was integration with other build systems. Should we cc folks from Facebook or Google maybe?

ehuss commented 4 years ago

Basically, we just include OUT_DIR to the build-script json message, as if it printed cargo:rustc-env:OUT_DIR=/foo/bar. This seems like something we should just do. Should I send a PR ? :)

If that works for you. I'm not familiar with how rust-analyzer or intellij works. I would maybe just add a new key to the structure (out_dir) instead of stuffing it in the env array (to avoid confusion about what the script sets vs what cargo sets).

Xanewok commented 4 years ago

cc @Xanewok , I think build plans play a role in the non-cargo story for RLS.

Yes, that's currently how our support for external build systems work. We parse the --build-plan output and do the compilation ourselves in the RLS.

FWIW I tried to address the build plan integration and (by accident) #7178 while working on Buck integration in the PR #6213.

To generate a detailed build plan (where you only need to run rustc) one had to run and parse the output of build scripts before a full, static build plan could be generated. The original motivation for that PR was to emit file dependencies per each unit but as it turns out that's also only available in the post-build-script phase, so that's what was implemented.

We agreed with @alexcrichton then that approach from #6213 seemed too 'bolted-on' and would require more effort to get it right. Maybe now would be a good time to design together a better way forward? Until then, I think it'd be good not to outright remove delete them (maybe we could deprecate/destabilize it?) until we know how to proceed.

alexcrichton commented 4 years ago

Thanks for all the responses everyone! With the merge of https://github.com/rust-lang/cargo/pull/7622 it sounds like we may be on good track to delete build plan support? I suspect that'll want to propagate through for a few weeks/cycles, but once all the tools switch over to that I believe we'll have no active users of the build plan support.

Xanewok commented 4 years ago

cc @jsgf

jsgf commented 4 years ago

I have a pretty solid tool for generating Buck build rules from Cargo, and build plans have played no part in that. I did try to use them, but found the information was at the wrong level of abstraction.

I'm fine with removing build plans and trying again from scratch.

alexcrichton commented 4 years ago

Ok I'm going to set myself a TODO for ~2 months from now to delete build plans. In the meantime we can update documentation and such and make sure to comment on issues that the intention is to delete this feature.

denzp commented 4 years ago

There is a tool, I'm actively developing, that uses build plans - cargo-wharf.

The tool is an integration layer for BuildKit build system and effectively is "cargo in docker". It uses the build plan feature to construct a BuildKit graph.

It didn't get much attention yet, but I believe in its potential since I think many people might use Rust for creating async scalable microservices now. This often involves containerization and Docker is a popular choice.

Interesting enough, the current implementation and its limitation are perfectly fit into my use case. For instance, the independent (from Cargo) build scripts running made possible to have a system-wide build script results caching.


On the other hand, I understand that there is no reason to keep an unstable feature that is not being widely used. Especially if its maintenance often makes a hard time for contributors. For quite a while, I was under the impression that Cargo uses build plans internally.

Would it possibly make sense to gradually make Cargo to first generate a build plan and then only to execute it? Right now it probably won't give any benefits, but in the future, it might lead to interesting possibilities. I have a feeling the refactoring might also improve maintainability and testability of the Cargo code.

alexcrichton commented 4 years ago

@denzp that was sort of the theory for build plans all along, but it never manifested. Build plans, as is today, are suitable to drive Cargo's own compilation. I think most here want to see a world where Cargo build plans exist, but the problem is that we're not there today and we don't have any manpower to actually make it a reality, so the team decided it's best to reflect reality and delete the build plan feature rather than let it linger and give the false impression that it will ever be stabilized near as-is.

tormol commented 4 years ago

I've used build plans to reduce compile times*. Using --build-plan to get the final rustc invocation seemed less brittle than parsing the output of cargo build --verbose, but as the mentioned issues show that turned out not to be the case. The introduced cache-busting forcing all dependencies to be recompiled made --build-plan useless for me, and I'm fine with it being removed as I have alternatives.

*: I was implementing code reloading for a game, and had noticed that running the final rustc command directly was slightly (but noticeably) faster than cargo build. I'm aware of having to run build scripts as I noticed missing linker flags. That was unexpected, but I implementing running them. (The time savings from reduced compile times certainly haven't paid off, but time passes faster when writing code than when waiting for other code to compile :wink:)

nirbheek commented 4 years ago

I've been planning (heh) on adding Cargo integration to Meson using the build-plan feature, so I was surprised and disappointed to hear that no one else has been working on it or using it actively.

However, I'm glad to hear that the Cargo team wants something like build-plans to exist, since if I do find time to work on Cargo integration in Meson in the future, I would like to pick this up again.

Thanks for the clear communication around this deprecation.

denzp commented 4 years ago

@alexcrichton would you recommend me to investigate and propose build-plans v2 and estimated steps how the Cargo internals can be "rebased" on top of a build plan? If that's something Cargo Team is still interested in, I'd like to give it a shot.

alexcrichton commented 4 years ago

@denzp we're still very interesting in having something done in this space, but unfortunately the cargo team doesn't have a lot of time to help out with it right now. We'd like to see iteration and development on build plans, though, so it's trying to find a good balance of getting this feature designed/implemented with not halting other work to focus all efforts on this.

That's unfortunately not a great answer, but if you're ok being pretty independent in investigating a "v2 build plans" that'd be best!

xclaesse commented 4 years ago

Disclaimer: I haven't followed the whole conversation here, and I'm not even a cargo/rust user. But as a Meson and GStreamer developer (not maintainer) I tried to have a plan to integrate with Cargo.

The big challenge is how do we pass dependency information from Meson to Cargo and vice-versa. I came to the conclusion that pkg-config's -uninstalled.pc are exactly that. With the Meson PR https://github.com/mesonbuild/meson/pull/4436 we can generate a -uninstalled.pc for each library built by Meson, and we can set PKG_CONFIG_PATH pointing to the directory where Meson gen them when invoking Cargo, and it will use them (it works with gst-plugins-rs).

The missing part is doing the other way around: Use libraries built by Cargo in Meson. For that we would need Cargo to be able to generate -uninstalled.pc files at configure time (that concept does not yet exist in Cargo, AFAIK). So I think adding in cargo a mode that would: 1) Download all crates 2) For each crate, tell them to generate their -uninstalled.pc files. 3) Return the path to the directory where it all got created.

That way, Meson could invoke that Cargo configuration step, then set PKG_CONFIG_PATH to the location of -uninstalled.pc files, so future dependency() calls in Meson will find them.

I'm sure there still would be rough edges, but that would be a good start.

chinedufn commented 4 years ago

@vlad20012 https://github.com/rust-lang/cargo/pull/7622

chmanchester commented 4 years ago

This is something we're actively interested in prototyping as a part of the Firefox build. We've had a version of the Firefox build driven by the build plan in the past, and plans for this quarter to prototype that as a part of the default build. Our motivation for this is the idea that a full view of the cargo build graph will help parallelize and schedule rust compilation within our build more efficiently. We've made some measurements in the past that suggest scheduling and parallelizing rust is a significant bottleneck for Firefox builds, and this seems like a promising way forward.

The shortcomings of the current build plan are significant, and impact our build -- hacking around some of the limitations mentioned here when I got this going last certainly didn't seem sustainable. However, prototyping things may prove gains sufficient to motivate investment in a future solution in this area.

None of this directly bears on plans to delete the current build plan implementation necessarily, as I believe we can do this prototyping work within the time constraint of versions of cargo that continue to support build plans, but we wanted to add this to the discussion in the meantime.

jsgf commented 4 years ago

@chmanchester We've been finding that by building Rust with Buck gets a lot more global parallelism, esp because it has a global dependency graph that includes all languages and transformations (codegen, etc). Also it has a more effective global caching strategy.

I've been having reasonably good success in semi-automatically deriving Buck build rules from Cargo; it certainly highlights the part of Cargo's design which are not well suited to deriving a global dep graph (primarily build scripts). But I think it's a promisiing approach for Cargo <-> Other integration that I'd like to continue to explore. But Cargo build plans as they currently stand don't help at all.

chmanchester commented 4 years ago

@jsgf, that's good to know -- I'd be very interested in learning more about your approach if you have a link to this tool.

jsgf commented 4 years ago

It's not public yet, but I'm planning on doing that soon.

boris-kolpackov commented 4 years ago

The Crate dependency discovery thread on internals.rust-lang highlights some parallelization (and maybe even build correctness) limitations of Cargo. I thought I would link this since it appears relevant to the discussion.

tmandry commented 4 years ago

cargo-raze is an interesting tool that works at what I'd consider to be the correct level of abstraction (it is literally built for integrating into another build system).

On Fuchsia we were discussing adapting cargo-raze for our build system (GN) and using that as a template for build plans v2, but we haven't had time to push that forward yet.

luser commented 4 years ago

cargo-raze is an interesting tool that works at what I'd consider to be the correct level of abstraction (it is literally built for integrating into another build system).

I was literally just leaving a comment on a cargo-raze issue that referenced the build plan output. cargo-raze currently uses cargo as a crate dependency which makes its build times very long and isn't ideal from anyone's perspective. Figuring out a path forward for the build plan work so that it meets the needs of tools like that and @jgsf's buck tool sounds ideal to me. (My current work environment has a Rust project built via cargo and also gradle and XCode in the mix and I'd love to replace it all with bazel TBH.)

jsgf commented 4 years ago

@luser My tool is in production use in house, and I've started the process to open source it. I think its output will likely work with Bazel as well (or wouldn't take much to make it work).

alexcrichton commented 4 years ago

I've put up https://github.com/rust-lang/cargo/pull/7902 to carry through the conclusion of this.

rickystewart commented 4 years ago

To follow up on @chmanchester's previous comment, we did carry out the prototype to integrate cargo --build-plan with the Firefox build. The proof of concept is hacky and not production-ready (even setting aside the fact that build plan support in cargo is apparently being deleted imminently), but what we're seeing so far is that using Cargo to fetch the build plans which we then convert into make recipes:

So even if we won't have the option to hitch our star to --build-plan, the results of this prototype suggest that something along these lines would be a very fruitful approach for optimizing the Firefox build in the future. If --build-plan is doomed in its current form then we on the build team at Mozilla would be interested in discussing other possible ways forward here, especially since a production-ready v2 of this feature seems generally useful and applicable to several different projects, like the Bazel and Buck tools mentioned above.

benbrittain commented 4 years ago

In the spirit of cargo-raze, we use a tool called cargo-GNaw for our builds now.

https://fuchsia.googlesource.com/fuchsia/+/refs/heads/master/tools/cargo-gnaw/

It consumes cargo metadata and outputs GN files. I'm not going to go so far as to call cargo-metadata the ideal way of doing this, but it is sufficient.

matklad commented 4 years ago

cc https://github.com/rust-lang/cargo/issues/8002 which might cover some of the plausible use-cases for build-plans.

UebelAndre commented 4 years ago

Should --build-plan be removed, what would the alternative be? I'm interested in using something like this in cargo-raze for gathering rustc flags/expected features and I think it'll greatly benefit the Bazel Rust rules as well. Perhaps one of the code owners from that project would be able to chime in here.

@acmcarther @mfarrugi @damienmg @smklein @dfreese

damienmg commented 4 years ago

The current approach I would prefer would be to step outside of build plans to use build manifest although this is a lot of work.

UebelAndre commented 4 years ago

@damienmg do build manifests currently contain all the necessary information? What is the outstanding work here? (If you don't mind 😅)

damienmg commented 4 years ago

Build plan contains all the information we need, manifest does not contains the version resolution, which is not the job of Bazel so we would have to have a tool to do it. Other than version resolution I don't see any issue that cannot be done on Bazel side (and the fact that parsing toml would need a custom tool).

tmandry commented 4 years ago

cargo-metadata does indeed have version resolution info. Docs are sparse but Resolve gets you a graph which you can use to look up resolved packages by ID. Each package has its version attached.

This is how cargo-gnaw works (linked above).

jsgf commented 4 years ago

BTW I did end up releasing my tool reindeer, which automatically (as possible) generates Buck build rules from Cargo dependencies.

Ericson2314 commented 3 years ago

Removing sounds good to me. I think a future version of this should be more actively dog-fooded by Cargo: in "normal mode" it just executes the plan it would spit out. That would of course require lot of work, but I really think that level of fidelity really is necessary to get decent value.


edit I've recently done some work on https://github.com/kolloch/crate2nix which does a nice job with just cargo metadata. I won't miss anything more that Cargo doesn't dogfood.


Relatedly, if build.rs got replaced with, say, build.ninja, it would be very interesting to merge that into the Cargo plan and get something nice and granular.

sxlijin commented 3 years ago

Out of curiosity, what exactly is the status of this? --build-plan still appears to be available in nightly, but it looks like intellij no longer depends on it- is the fact that it's still in the codebase a consequence of no one having gotten around to cleaning it up?

LukeMathWalker commented 3 years ago

The holy grail for production users is fine-grained caching of dependencies with reproducible builds.

I have been trying to find some solutions in this space for a while - cargo-chef is an attempt that plays nicely with Docker, but the cache is busted every time one crate changes in the dependency tree. The implementation is a glorified hack, trying as hard as possible not to re-implement cargo logic outside of cargo. Other projects, like cargo-guppy, explicitly mention re-implementing cargo features outside of cargo itself - see here.

I spent the last couple of weeks exploring alternative nix-based build systems for Rust projects (the various crate2nix, cargo2nix, naersk, etc.) - they are all, to various degrees, trying to re-implement cargo outside of cargo, based on partial information (cargo metadata, usually).

The only way to have a robust integration point for other build systems would be to ensure that cargo is actually using the same set of information to execute than it is providing to those other build systems - dog-fooding it, as @Ericson2314 mentioned. Is --unit-graph supposed to be the replacement of build-plans? Can it be relied upon, or is there a risk to see "diverging" builds because it is missing some information?

It would be great to understand, as an outsider, what is the "current" attempt at improving support for alternative build systems - I'd be more than happy to contribute, but at the moment I can't wrap my head around where this sits in terms of priority for the cargo team and what would be the best way to engage.

jsgf commented 3 years ago

@LukeMathWalker We're using reindeer in production with good success. It uses cargo to do version resolution and vendoring, then uses the output of cargo metadata to generate a complete set of build rules for Buck (which I suspect would very nearly work with Bazel, gn and similar build systems).

I initially tried using build plans but they're just at the wrong level. I don't want a trace of what cargo would do, but a more declarative description of what needs to be done not how to do it.

The main blocker here are build scripts since they can do unconstrained things with undescribed dependencies.

tmandry commented 3 years ago

We use a very similar approach for gn: cargo-gnaw along with developer scripts to update vendored crates using cargo's version resolution.

celinval commented 1 year ago

I do have a use case in mind that I was hoping could be accommodated before removing build-plans. We have extended Rustc to generate other kinds of artifacts and we use the same name schema as Rustc, which includes the metadata suffix.

AFAIK, build-plans is the only way we can reliably figure out the name of the files that we generate, especially when cargo build finds that everything is fresh. In that case, our compiler doesn't get invoked, and there is no easy way for us to tell which artifacts we should be using.

We tried to extract the artifact name from the artifact messages, but for things like binaries, static / shared libraries, the list of filenames do not include the original files that were generated by the compiler.

E.g.: For a static library, this is what I get:

"filenames":["/tmp/test/static_lib/target/debug/libstatic_lib.a"]

while for a rust library, I get:

"filenames":["/tmp/test/libA/target/debug/liblibA.rlib","/tmp/test/libA/target/debug/deps/liblibA-728fd0386e8e49d7.rmeta"]

On the other hand, cargo build --build-plan includes the name of the file:

"outputs": ["/tmp/test/static_lib/target/debug/deps/libstatic_lib-edb6526891c85cb2.a" ],