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

Should build-plans be deleted? #7614

Open
alexcrichton opened this issue Nov 21, 2019 · 14 comments
Labels

Comments

@alexcrichton
Copy link
Member

@alexcrichton alexcrichton commented Nov 21, 2019

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:

  • Transitively passing arguments from build scripts to later crates
  • Encoding information about pipelining
  • Requires implementors to have their own parsing of build script outputs and such

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 #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 #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

This comment has been minimized.

Copy link
Contributor

@CAD97 CAD97 commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

@Manishearth Manishearth commented Nov 21, 2019

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

@vlad20012

This comment has been minimized.

Copy link
Member

@vlad20012 vlad20012 commented Nov 21, 2019

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

This comment has been minimized.

Copy link
Member

@matklad matklad commented Nov 21, 2019

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:

  • OUT_DIR and build.rs produced cfgs should be communicated to the IDEs somehow
  • build plan seems to be a conceptually wrong way to do this, +1 to removal (unless 3rd party build system integration benefits from build plans)
  • I don’t know how a good solution for IDE problem should look like, current cargo semantics make this non-trivial
  • given that IntelliJ already needs OUT_DIR, and rust-analyzer will need it “soon” (soon — someone has a spare weekend) it might make sense to prototype some replacement for the narrow IDE use-case. It’s ok if it’s nightly only.
@matklad

This comment has been minimized.

Copy link
Member

@matklad matklad commented Nov 22, 2019

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: #7546 (comment). 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: #7622

@matklad

This comment has been minimized.

Copy link
Member

@matklad matklad commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Contributor

@ehuss ehuss commented Nov 22, 2019

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

This comment has been minimized.

Copy link
Member

@Xanewok Xanewok commented Nov 24, 2019

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

This comment has been minimized.

Copy link
Member Author

@alexcrichton alexcrichton commented Nov 25, 2019

Thanks for all the responses everyone! With the merge of #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

This comment has been minimized.

Copy link
Member

@Xanewok Xanewok commented Nov 26, 2019

cc @jsgf

@jsgf

This comment has been minimized.

Copy link
Contributor

@jsgf jsgf commented Nov 27, 2019

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

This comment has been minimized.

Copy link
Member Author

@alexcrichton alexcrichton commented Dec 2, 2019

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

This comment has been minimized.

Copy link

@denzp denzp commented Dec 4, 2019

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

This comment has been minimized.

Copy link
Member Author

@alexcrichton alexcrichton commented Dec 4, 2019

@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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
9 participants
You can’t perform that action at this time.