-
Notifications
You must be signed in to change notification settings - Fork 1.7k
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
Add support for cargo subprojects #11856
Conversation
xclaesse
commented
Jun 8, 2023
•
edited
Loading
edited
- interpreter: Add support for cargo subproject
- cargo: Fix '1.0.45' version conversion
- cargo: Support all crate library types
- cargo: Package description is optional
- cargo: Add support for rust_dependency_map
- cargo: Use "-rs" suffix convention to lookup for Rust dependencies
- cargo: interpreter: Reorganize functions order
- cargo: Remove unused function
- cargo: Builder: Add dict support
- cargo: builder: Remove all duplicated functions
This PR is based on top of #11730. |
930c3f0
to
be44e8c
Compare
Looking at this I was wondering what's the user story here for just pulling in crates into your meson project. Like if all your rust targets are declared within the normal meson DSL, could the support for external crates be done in a way not requiring wrap files? Like just creating normal E.g.: |
Currently you still would need to have wrap files pointing to crates.io. I experimented with generating those wraps automatically though: #11727.
That's not a bad idea actually. We are not there yet, but I'll keep that in mind for future improvement. |
cfa65fe
to
bfba8e6
Compare
876cb75
to
3cd7cf5
Compare
3cd7cf5
to
dbcb8b7
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm a little dubious of the first patch. I had hoped to eventually be able to use the builder API with cmake, but the Builder
class assumes that you're parsing a single file (a useful abtraction for Rust where it's true), but obvious not true for other cases. I guess we could revert later, but I'm not sure how I feel about it.
methods_map: T.Dict[wrap.Method, T.Callable[[str, str, T.Dict[OptionKey, str, kwtypes.DoSubproject]], SubprojectHolder]] = { | ||
'meson': self._do_subproject_meson, | ||
'cmake': self._do_subproject_cmake, | ||
'cargo': self._do_subproject_cargo, | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It would be better if this was stored to the instance in __init__
or as a class variable, otherwise this dict can be recreated on each function call, but I think this is a nice cleanup
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
To be honest, I don't think creating a dict with 3 items is going to change anything to perf, and it's not in a critical path. I prefer keeping that map definition close to where it's used for readability.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
timeit
says that it cuts the performance of a function almost in half to initialize a 3 member dict inside a method vs putting it in class, but I guess we can leave it for now and improve it later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I don't think the map needs to be close by for readability, that's what variable names are for.)
Keep only the Builder class, there is no point in duplicating everything.
You can easily have one Builder instance per file. The filename is actually the only state that class has. I don't know how cmake currently builds its AST, but would make a lot of sense to share the code indeed. But let's cross that bridge when we get there IMHO. |
dbcb8b7
to
8ccf24f
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks for taking this up and getting it here. I'll plan to have a look at part 2 tomorrow.
I assume this would work recursively, ie dependencies from the The other concern I have is the When it's considered impossible to support I sincerely appreciate the effort here and hope that these last couple of issues can be solved. |
Yes that's already the case.
My general feeling regarding I think a big part of the solution is Cargo should be improved to not require
There is system-deps for that. We have a PR to add support for that in Meson. Everybody would benefit from using it instead of adding ugly hacks in their
I haven't done much Rust code, but from what I saw it's pretty much always something like adding
My understanding is they would need to be compiled and run at configure time, which is pretty bad design. What happens if a |
Fantastic, I'll have to play with it a bit.
Yeah, that's the misconception I was trying to address. It's definitely used for that and to be fair, for a crate the needs a system library, there's really no other way to locate it. I do agree that it would be nice if there was a better and more integrated and declarative way for cargo to do that.
I guess I haven't really looked into a lot of
Indeed, and that's a feature and it's no different than the dependency resolution you already do for the main build target - it's just for the host. It's very powerful to be able to use the same crate ecosystem for |
I have plans to write something like system-deps to allow writing platform and compiler based checks in cargo, using the same pattern. codegen is the one of the other things that I know of, along with c/c++ compilation, and those are the things we are very hard to support in a way that is pleasant for our users without changes from rust and/or cargo |
If you can already depsolve and build rust binaries and their dependencies, you should be able to do that for |
The dependency graph isn't the hard problem though, build.rs is a black box escape hatch that can do just about anything, and since it's rust language we can't realistically introspect it. I spent a lot of time thinking about build.rs and here's all of the questions I came up with after reading a b This means:
For the case if 2, if we can solve all of those issues above, then we can handle it as a normal code generator, and (once my PR to have build machine subprojects lands) we can run build.rs as a normal target. Case 1 on the other hand would be very difficult to come up with a reasonable solution for. It would, I think, require us to generate a build.ninja for each build.rs and run that at configure time to capture the --cfg flags, and that's going to make configure slow as mud. And that doesn't even begin to deal with the problem of what if build.rs does both things. The only realistic solutions are going to require changes to the Cargo manifest either way. Either through the use of system-deps (and similar yet unwritten tools), or through metadata annotations to tell Meson how to use build.rs as a code generator. |
I think this is missing the pragmatic in my previous reply. The ideal approach may be to introspect Most of the concerns you mention seems like something that can either be parsed from the stdout of running the build binary or you can require the user to specify the output files (for the top level dependency only) like you have to for a custom_target. Every dependency below the top-level crate dependecny can work like cargo does. If having to specify outputs and their install location is what you're thinking of when you say "a way that is pleasant for our users", then I really think that's a small price to pay for this to work. It's not that different from integrating a python script as a custom_target. Maybe we're getting hung up on the fact that in some sense, that information is available in the |
I'm not talking pragmatic, I'm talking about working. Sure, we can successfully run a build.rs with no dependencies to extract --cfg flags at configure time, if we want to. That's not super hard to solve. Codegen is, however, quite hard to solve:
I'm not trying to be an idealist here, I just see serious issues with no easy solutions. |
I feel like this is a misunderstanding of both what a build.rs is and what a meson is. build.rs is a way to run "fully arbitrary code at configure time". But what it's actually for is to perform "configuration" and "custom build steps" and feed those as prerequisite targets to rustc.
Well, of course there's another way to locate it. The issue is specifically that cargo is the top and bottom third of a build system, but not the middle. It can only declaratively define a build, but then people also want to script their build and do things other than "invoke the rustc compiler on all source files". meson can locate it, cmake can locate it, autotools can locate it, and cargo's system-deps addon can locate it. All are better options than "compile a program called build.rs, and expect it to print out the relevant information".
Right, which is why if one is using meson, it seems to me like it makes more sense to use meson's wayland module or else just a custom_target for. Yes, this does mean one needs to port a build.rs over to equivalent idioms in meson.build.
Wouldn't parsing wayland protocol XML be a build step, not a configure step. You don't need to parse XML in order to concretize the build graph and define input/output rules. You just need to parse XML in order to emit build target outputs. Meson fully allows you to compile a crate into an executable program at build time, then use that program as the compiler executable that compiles XML files into additional source files ( What you cannot do at configure time is run a |
Ultimately, I don't think "people wrote a build system in build.rs and now other people need to translate that build system into a direct port as a meson.build idiom" is a violation of pragmatism. Because people should do that, it's going to be better, faster, and more reliable. This is a reason to avoid build.rs even without taking into account the fact that as @dcbaker observed, it is literally impossible to execute build.rs at configure time (before building the crates at build time which build.rs depends on!!!), have it do completely arbitrary things, and then have a blackbox build system bottleneck the entire build process in both directions.
But build.rs should be deprecated and replaced. It will result in a better, faster, and more reliable experience for users of Deprecating and replacing bad uses of build.rs (like system-deps) is a net positive to the And this can be automatically parsed by meson's cargo subprojects parser into This, plus a better way to handle "check the name of the operating system and pass a --cfg", would vastly reduce the number of actual build.rs files that exist, lower the cognitive burden of translating the remainder into meson.build idioms, and speed up everyone's |
When running
Generally, the artifacts that are generated by the build script are placed in the directory specified by the env variable
Otherwise, if any output from
Is there anything not covered by the suggestions above?
I think we can figure this out together. |
Your suggestions above were the inputs that resulted in an output of half of the meson developer team saying "this is not practically feasible to do" and the other half saying "this is bad and we should not do it even if we could". In particular, you haven't addressed the major points which @dcbaker raised regarding why it is not practically feasible. Please reread his arguments. I would prefer not to rehash the same discussion multiple times. |
You're not doing the meson project any favors here. |
Okay, I thought about this and we might be able to solve the problem of the dependencies with Ninja's dyndeps. It would be preferable to me to not have to wrap the build.rs output in a separate script to create the dyndep file (ie, it would be great if the build.rs generated this itself), it makes things slow, especially on Windows. So us having to wrap the build.rs call to read
Ninja does rule ordering based on file outputs, not on rules themselves. This is advantageous because if a generator outputs multiple files, but a consumer only uses one of them, it doesn't trigger a rebuild if the generator is re-run, but doesn't update the file that target consumes. Consider that you have a lib and a bin, and your generator creates two outputs, one that only the lib uses and one that only the bin uses. If the generator is re-run, and only touches the file for the bin, then the lib rule isn't re-run. So, in the simplest case "there is one lib, and the build.rs generates things for the lib" then we can do this. If there are more than one build target in the crate we're doing pessimal build ordering, in the worst case blocking the entire build for the generation of test data or a pkg-config file. I think this is still a major sticking point for us. |
So, let me summarize where I think we are on this:
|
No idea how cargo handles this, but I don't immediately see how it could be doing better in such a case. My expectation for meson would be to do about as well as cargo would. If it's painfully slow in cargo, chances are the crate's devs have already split the crate in two.
Did you come to that conclusion simply because libc tries to run In any case, a build script that breaks while cross compiling would be a bug of the respective crate and obviously not meson's concern.
Similar vein here, it's obvious that meson cannot make every insanity work. Most build scripts compiling C code would hopefully use the
So, dependencies here pose only a performance problem? Because slow support for build scripts would still be vastly better than the current status of no support at all. The compiled dependencies should obviously be cached (just like the final compiled build script) and thus hopefully not cause notable slowdown on future configuration runs. |
We make a strict guarantee that if you put a binary in the binaries section we will use it. It's an absolute disservice to our users to lie to them, and it's going to be an endless source of bugs "Why is it not using the c compiler in my cross file!" This is a place where I'm willing to draw a line and say absoutely not
This is even worse, actually. We do a lot of work behind the scenes to make bindgen (and I'm working on cbindgen as well) behave nicely with Meson and ninja. Setting correct cflags, stds, adding the necessary flags so that clang will generate a depfile, etc. I also have patches to make bindgen automatically do the right thing with Meson's idea of what a C++ header is. I would be 100% opposed to switching from using standalone bindgen to the crate version in mesa, and in general it's a bad idea, this is 100% against Meson's philosophy of "don't make everyone write the same code 1000 times, every case will be subtly wrong." Regardless, it would be impossible to get 100% overlap in features, since there are a number of opaque types involved and I don't think we'll be making those types transparent. I'll send a patch to add support for version constraining bindgen, which should help with the case of outdated bindgen, and I'm opened to creating a configure time test for bindgen if we can identify a good way to test that the binary works. |
Ok, I guess this is where I feel that meson could be more pragmatic. If this is representative for how the meson projects feels, at least this is a good reference for why meson wont have useful integration with cargo. |
fwiw, I am in favor of doing all of the following:
In any case, I don't think there is a cause for panic if Meson isn't perfectly interoperable with every |
Do we really have to go there? You're really not doing the rust community any favors here by coming multiple years late to the party, telling us we're doing it all wrong, and advising us that we ourselves should put in the work to try an approach we couldn't get to work and have since given up on. It comes across as highly reminiscent of a very particular meme about how rust users interact with non-rust users. I don't know whether that was your intention, but I can absolutely confirm that was the result. Additionally we've gotten a clear message from the rust leadership that they are not interested in this problem and that despite rfcs and unstable features that tried to help solve the problem, those features don't work and can't be stabilized and no one is motivated to drop the important work for which they are actual stakeholders, that they are doing to advance the rust programming language, in order to work on cargo features that don't affect cargo users. I don't blame them. And you should not blame us. Gluing together multiple build systems is hard and finicky at the best of times, just like any other two things that both work very differently and were never originally specced to plug in to each other. This is why interfaces like installable libraries and pkg-config files, is important. ... A solution that covers barely even the tip of the iceberg of "things that build.rs may do", and only does so when build.rs doesn't have external dependencies, and only does so when you don't cross compile, is frankly not a good or even a halfhearted solution. Again, the absolute minimum we require is that at configure time, without the aid of a command runner such as ninja, we somehow acquire a build plan (and only a build plan!) that allows us to run the entire meson build in the correct order, with the correct flags, including --cfg. And it must be parallelizable, e.g. if build.rs emits multiple outputs and a build edge only needs one of them, it shouldn't have to wait for what it isn't using via a global lock. There's certainly a subset of build.rs files that fulfill this contract. It's the ones that use solely the rust stdlib (and without running subprocesses) in order to detect a series of --cfg flags to use, and do nothing else. Detecting that case from meson without including a rust source code parser and without strace (😂) sounds... adventurous. But also this is exactly the scenario where porting to meson.build takes at most 3 minutes. ... My understanding is that this doesn't even affect the common case, so it's not clear to me why so much philosophical fuss about a practical non-issue. Just have someone port the project to the wrapdb and zero people will ever have to think about it again. |
Absolutely. Stuff like system-deps can help tremendously on the cargo side too -- cargo users do not love build.rs and want a declarative build anyway. Declarative builds are fantastic -- when the project is simple enough that it doesn't need anything else, which has always been the bugbear of build systems.
👍 This was the original plan anyway. |
That has always been my plan. This PR #11867 allows having |
This is close to the position I'd like as well. Allowing build.rs to work for cases were we can reasonably support it reliably and correctly is great. Let's do that. Cases where we can't get correctness and reliability (or the performance is really bad) let's figure solutions out. I'm willing to put in the work on both sides to make all of this work as much as possible |
FWIW, I do not endorse this message, and I do not agree with it. It is an unnecessarily escalatory (and generally unnecessary) response to an unproductive comment. Individual disgruntlement or irritation does not represent any particular community, and I hope people will stop posting angry comments that have the potential to derail the important work being done in Cargo ↔ Meson integration. |