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

Add support for cargo subprojects #11856

Merged
merged 11 commits into from
Oct 10, 2023
Merged

Conversation

xclaesse
Copy link
Member

@xclaesse xclaesse commented Jun 8, 2023

  • 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

@xclaesse
Copy link
Member Author

xclaesse commented Jun 8, 2023

This PR is based on top of #11730.

@xclaesse xclaesse force-pushed the cargo-subproject branch 9 times, most recently from 930c3f0 to be44e8c Compare June 10, 2023 16:53
@xclaesse xclaesse added this to the 1.2.0 milestone Jun 10, 2023
@karolherbst
Copy link
Contributor

karolherbst commented Jun 11, 2023

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 dependency objects with a special crates-io type or something and meson just deals with all of this internally?

E.g.: dependency('syn', version : '>=0.50', method : 'crates-io')

@xclaesse
Copy link
Member Author

Currently you still would need to have wrap files pointing to crates.io. I experimented with generating those wraps automatically though: #11727.

E.g.: dependency('syn', version : '>=0.50', method : 'crates-io')

That's not a bad idea actually. We are not there yet, but I'll keep that in mind for future improvement.

@xclaesse xclaesse mentioned this pull request Jun 12, 2023
@dcbaker dcbaker modified the milestones: 1.2.0, 1.3.0 Jun 28, 2023
@xclaesse xclaesse force-pushed the cargo-subproject branch 3 times, most recently from cfa65fe to bfba8e6 Compare August 6, 2023 16:53
@xclaesse xclaesse force-pushed the cargo-subproject branch 2 times, most recently from 876cb75 to 3cd7cf5 Compare September 8, 2023 00:01
Copy link
Member

@dcbaker dcbaker left a 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.

mesonbuild/cargo/builder.py Outdated Show resolved Hide resolved
mesonbuild/cargo/interpreter.py Outdated Show resolved Hide resolved
mesonbuild/cargo/interpreter.py Show resolved Hide resolved
mesonbuild/cargo/interpreter.py Outdated Show resolved Hide resolved
Comment on lines +931 to +935
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,
}
Copy link
Member

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

Copy link
Member Author

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.

Copy link
Member

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.

Copy link
Member

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

mesonbuild/interpreter/interpreter.py Show resolved Hide resolved
Keep only the Builder class, there is no point in duplicating
everything.
@xclaesse
Copy link
Member Author

xclaesse commented Oct 9, 2023

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

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.

Copy link
Member

@dcbaker dcbaker left a 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.

@xclaesse xclaesse merged commit 3af0632 into mesonbuild:master Oct 10, 2023
37 checks passed
@xclaesse xclaesse deleted the cargo-subproject branch October 10, 2023 01:13
@krh
Copy link

krh commented Oct 11, 2023

Currently you still would need to have wrap files pointing to crates.io. I experimented with generating those wraps automatically though: #11727.

E.g.: dependency('syn', version : '>=0.50', method : 'crates-io')

That's not a bad idea actually. We are not there yet, but I'll keep that in mind for future improvement.

I assume this would work recursively, ie dependencies from the Cargo.toml from syn would be turned into meson dependencies? If so, that would be wonderful.

The other concern I have is the build.rs elephant in the room. I get that there's a bit of frowning on using build.rs to discover system dependencies, which is understandable since that's a big part of what meson does. But there is so much more that build.rs is used for that doesn't step on meson's toes and that meson can't and shouldn't absorb. And frankly, it's a bit unfortunate that bindgen has been special cased, since that's really just the tip of the iceberg for code generation. It's a level of meta-programming that's integral to the rust developer experience and I don't think it's practical to not support it.

When it's considered impossible to support build.rs is that from a point of view that meson should parse it and convert build.rs dependency discovery to meson native discovery? Because yes, that doesn't seem tractable. But I think that a pragmatic approach of building it, running it and parsing the cargo directives it outputs should be feasible. Has this been considered?

I sincerely appreciate the effort here and hope that these last couple of issues can be solved.

@xclaesse
Copy link
Member Author

I assume this would work recursively, ie dependencies from the Cargo.toml from syn would be turned into meson dependencies? If so, that would be wonderful.

Yes that's already the case.

The other concern I have is the build.rs elephant in the room.

My general feeling regarding build.rs is they are really bad hacks to work around Cargo limitations and missing features.

I think a big part of the solution is Cargo should be improved to not require build.rs that much. In the meantime, the stopgap solution I proposed is to let rust projects add a custom meson.build file that gets included by the generated Meson project: 34f7b88.

I get that there's a bit of frowning on using build.rs to discover system dependencies

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 build.rs.

But there is so much more that build.rs is used for that doesn't step on meson's toes and that meson can't and shouldn't absorb.

I haven't done much Rust code, but from what I saw it's pretty much always something like adding --cfg flags depending on the platform. That definitely can be done much better in Meson.

But I think that a pragmatic approach of building it, running it and parsing the cargo directives it outputs should be feasible. Has this been considered?

My understanding is they would need to be compiled and run at configure time, which is pretty bad design. What happens if a build.rs uses other crates, they also need to be built at configure time?

@krh
Copy link

krh commented Oct 11, 2023

Yes that's already the case.

Fantastic, I'll have to play with it a bit.

My general feeling regarding build.rs is they are really bad hacks to work around Cargo limitations and missing features.

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 haven't done much Rust code, but from what I saw it's pretty much always something like adding --cfg flags depending on the platform. That definitely can be done much better in Meson.

I guess I haven't really looked into a lot of build.rs files across crates, but I find myself using it for codegen a lot. Maybe I like codegen too much, but aside from the common bindgen for generating rust code for C libraries, there's parsing Wayland XML and generating protocol code, parsing an instruction set spec and generating disassembly... it's everything that you'd use a meson generator or custom_target for.

My understanding is they would need to be compiled and run at configure time, which is pretty bad design. What happens if a build.rs uses other crates, they also need to be built at configure time?

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 build.rs. For example, if you are parsing Wayland protocol XML to generate code, you need to pull in an XML parser crate, which could be the same crate that you are using in the main build target, but built for the host, of course. You wouldn't write a code generator in python to use with meson and then not allow using external python modules.

@dcbaker
Copy link
Member

dcbaker commented Oct 11, 2023

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

@krh
Copy link

krh commented Oct 11, 2023

If you can already depsolve and build rust binaries and their dependencies, you should be able to do that for build.rs. I can see that it's irritating and against meson principles that build.rs conflates configure time (for locating deps) and build time (for codegen uses). But from a pragmatic point of view, that's how cargo works and I don't think meson can or should solve that.

@dcbaker
Copy link
Member

dcbaker commented Oct 11, 2023

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
Does it generate files? does it print cargo cfg flags? does it download and compile c libraries? Some combination? What files? where does it put them? do they need to be installed? do they need to to be structured in a specific way to be used? do they have ordering constraints?

This means:

  1. cfg flags must be known at configure time, because they must be written into the ninja.build file. For this case we must run build.rs at configure time to have those flags
  2. any codegen must be done through the backend (ninja, vs, xcode, etc). So now build.rs has to be run at build time

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.

@krh
Copy link

krh commented Oct 11, 2023

I think this is missing the pragmatic in my previous reply. The ideal approach may be to introspect build.rs and extract everything it does into meson dependency checks, meson generators, install rules etc. But as you say, that's not practical, and it's also not practical to expect build.rs to be deprecated or replaced by a handful of fixed function declarative configration points.

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 build.rs and the user shouldn't have to specify that again, I think this is perfect being the enemy of good enough.

@dcbaker
Copy link
Member

dcbaker commented Oct 11, 2023

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:

  • We are unaware of the inputs that build.rs needs to read for its outputs. The only solution is that mark them as always dirty, or to annotate the Cargo.toml with metadata. If we mark it always dirty then that propagates and every target that consumes a build.rs output which will have to be rebuilt and re-linked on every call to ninja, that's a bad user experience.
  • We are unaware of the outputs that build.rs generates, and where it will put them. The only solution is to annotate Cargo.toml. And we have to have this, because otherwise ninja wont order things correctly, and it wont rebuild the right things if the cargo subprojects are updated, and that's also a bad user experience.
  • We are unaware of how those sources are going to be used. If the files need to be used in anyway except directly from the root of the build directory we must know that, and we need some way to communicate that.

I'm not trying to be an idealist here, I just see serious issues with no easy solutions.

@eli-schwartz
Copy link
Member

The other concern I have is the build.rs elephant in the room. I get that there's a bit of frowning on using build.rs to discover system dependencies, which is understandable since that's a big part of what meson does. But there is so much more that build.rs is used for that doesn't step on meson's toes and that meson can't and shouldn't absorb. And frankly, it's a bit unfortunate that bindgen has been special cased, since that's really just the tip of the iceberg for code generation. It's a level of meta-programming that's integral to the rust developer experience and I don't think it's practical to not support it.

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.

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.

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

I guess I haven't really looked into a lot of build.rs files across crates, but I find myself using it for codegen a lot. Maybe I like codegen too much, but aside from the common bindgen for generating rust code for C libraries, there's parsing Wayland XML and generating protocol code, parsing an instruction set spec and generating disassembly... it's everything that you'd use a meson generator or custom_target for.

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.

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 build.rs. For example, if you are parsing Wayland protocol XML to generate code, you need to pull in an XML parser crate, which could be the same crate that you are using in the main build target, but built for the host, of course. You wouldn't write a code generator in python to use with meson and then not allow using external python modules.

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 (.c or .rs). The name of the XML file needs to be known at configure time, and the filenames of the source code files that are compiled into wayland_proto_generator or whatever.

What you cannot do at configure time is run a build.rs code to read an XML file containing the list of wayland protocol files that need to be scanned and built but I do not think you were suggesting to do this. If someone did want to do this then my opinion on the matter is that one should simply not represent lists of source code files in XML and if you do then that is not meson's problem to solve.

@eli-schwartz
Copy link
Member

I think this is missing the pragmatic in my previous reply. The ideal approach may be to introspect build.rs and extract everything it does into meson dependency checks, meson generators, install rules etc. But as you say, that's not practical,

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.

and it's also not practical to expect build.rs to be deprecated or replaced by a handful of fixed function declarative configration points.

But build.rs should be deprecated and replaced. It will result in a better, faster, and more reliable experience for users of cargo build, since build.rs is slow and a bad experience and people like to avoid them if they can.

Deprecating and replacing bad uses of build.rs (like system-deps) is a net positive to the cargo build experience, because it teaches cargo how to directly do something without build.rs that people would like to do, and it allows cargo to do it in a declarative, straightforward, machine-parseable way that is easier for users to write and easier for cargo to execute.

And this can be automatically parsed by meson's cargo subprojects parser into dependency() function calls in meson.build.

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 cargo builds, that last case also helping to reduce outsize impacts on the earth's environment.

@krh
Copy link

krh commented Oct 16, 2023

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:

  • We are unaware of the inputs that build.rs needs to read for its outputs. The only solution is that mark them as always dirty, or to annotate the Cargo.toml with metadata. If we mark it always dirty then that propagates and every target that consumes a build.rs output which will have to be rebuilt and re-linked on every call to ninja, that's a bad user experience.

When running build.rs, it prints these things on stdout, for example `rerun-if-changed.

  • We are unaware of the outputs that build.rs generates, and where it will put them. The only solution is to annotate Cargo.toml. And we have to have this, because otherwise ninja wont order things correctly, and it wont rebuild the right things if the cargo subprojects are updated, and that's also a bad user experience.

Generally, the artifacts that are generated by the build script are placed in the directory specified by the env variable OUT_DIR and are only for use by the crate where build.rs comes from. These files are typically included in the rust code using the include!() macro, which is understood by rustc, which will list the included files in the dep-info output. For example:

example.d: example.rs more_stuff.rs stuff.rs

example.rs:
more_stuff.rs:
stuff.rs:
krh@imperial-needle:~/deps$ cat example.rs
mod more_stuff;

include!("stuff.rs");

krh@imperial-needle:~/deps$ cat more_stuff.rs
pub const GREETING: &str = "Hello, world!";
krh@imperial-needle:~/deps$ cat stuff.rs
fn main() {
    writeln!(more_stuff::GREETING);
}
krh@imperial-needle:~/deps$ rustc --emit=dep-info example.rs 
krh@imperial-needle:~/deps$ cat example.d
example.d: example.rs more_stuff.rs stuff.rs

example.rs:
more_stuff.rs:
stuff.rs:

Otherwise, if any output from build.rs is to be used by the meson side of the build system, I think it's fair to require that the meson user lists those outputs in the meson rule, similar to how outputs are listed for a custom target (including their install location, if any).

  • We are unaware of how those sources are going to be used. If the files need to be used in anyway except directly from the root of the build directory we must know that, and we need some way to communicate that.

Is there anything not covered by the suggestions above?

I'm not trying to be an idealist here, I just see serious issues with no easy solutions.

I think we can figure this out together.

@eli-schwartz
Copy link
Member

Is there anything not covered by the suggestions above?

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.

@krh
Copy link

krh commented Oct 16, 2023

Is there anything not covered by the suggestions above?

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.

@dcbaker
Copy link
Member

dcbaker commented Oct 16, 2023

When running build.rs, it prints these things on stdout, for example `rerun-if-changed.

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 cargo:rerun-if-changed= and write the dyndep file is not ideal. So lets call this one "there may a workaround, but there is room for improvement".

Generally, the artifacts that are generated by the build script are placed in the directory specified by the env variable OUT_DIR and are only for use by the crate where build.rs comes from. These files are typically included in the rust code using the include!() macro, which is understood by rustc, which will list the included files in the dep-info output. For example:

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.

@dcbaker
Copy link
Member

dcbaker commented Oct 16, 2023

So, let me summarize where I think we are on this:

  • build.rs files with dependencies are problematic for configure time, but if there are no dependencies we could consider running them at configure time for --cfg flags.
    • I have some concerns here because I've noticed a number of build.rs files are not safe for cross compilng (libc for example, assumes that if you're targeting freebsd you're running on freebsd)
    • I've also seen a number of cases where it would be difficult for us (Meson) to ensure that the correct programs are being used (ie, if a user puts a specific bindgen binary in their native or cross file we need to ensure that build.rs uses that binary).
    • if there are dependencies that's problematic, and this is still less than ideal because we really want to make the configure
      phase as fast as possible
  • for build.rs files used to generate code we may be able to use dyndeps to handle the file dependencies for rebuilds
    • this likely requires a wrapper script, which will make everything slow
  • for build.rs generators we don't have a good solution for mapping "what outputs are created" or "which targets consume them"
    • We could use stamp files, but that likely requires more wrapping and is a big hammer that may impact build times.
    • we could annotate cargo.toml
  • we also don't have a solution for what to do about build.rs files that both generate files and emit --cfg options. We would need a switch to ensure that only --cfgs are generated at configure time, and one to ensure that only generation is done at build time.

@LingMan
Copy link

LingMan commented Oct 16, 2023

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.

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.

I have some concerns here because I've noticed a number of build.rs files are not safe for cross compilng (libc for example, assumes that if you're targeting freebsd you're running on freebsd)

Did you come to that conclusion simply because libc tries to run freebsd-version in its build.rs or is there something else? Because that's only to select the exact ABI version on their CI. Any normal build - cross or native - uses the freebsd11 ABI.

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.

I've also seen a number of cases where it would be difficult for us (Meson) to ensure that the correct programs are being used (ie, if a user puts a specific bindgen binary in their native or cross file we need to ensure that build.rs uses that binary).

Similar vein here, it's obvious that meson cannot make every insanity work. Most build scripts compiling C code would hopefully use the cc crate, which reads several environment variables which people (and in this case meson) can use to configure which exact programs to use. At least CC, CFLAGS, and AR sound useful to meson, but I've only skimmed cc's readme.
For bindgen I'd expect that vast majority of crates to not use the binary but the normal crate. If possible I'd love to switch mesa to the crate version of bindgen, too. A good chunk of the rusticl bug reports could be summed up with "broken/outdated bindgen installation".

if there are dependencies that's problematic, and this is still less than ideal because we really want to make the configure phase as fast as possible

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.

@dcbaker
Copy link
Member

dcbaker commented Oct 16, 2023

Similar vein here, it's obvious that meson cannot make every insanity work.

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

For bindgen I'd expect that vast majority of crates to not use the binary but the normal crate. If possible I'd love to switch mesa to the crate version of bindgen, too. A good chunk of the rusticl bug reports could be summed up with "broken/outdated bindgen installation".

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.

@krh
Copy link

krh commented Oct 17, 2023

Similar vein here, it's obvious that meson cannot make every insanity work.

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

For bindgen I'd expect that vast majority of crates to not use the binary but the normal crate. If possible I'd love to switch mesa to the crate version of bindgen, too. A good chunk of the rusticl bug reports could be summed up with "broken/outdated bindgen installation".

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.

@nirbheek
Copy link
Member

fwiw, I am in favor of doing all of the following:

  1. Doing amazing hacks to make build.rs work as-is in as many cases as feasible
    • Alternatively, ensure that the really-difficult-to-port bits of build.rs (like codegen) are supported, and require a different mechanism for the rest (sysdeps, meson.build, etc)
  2. Providing people with very good alternatives to build.rs
    • Both on the Meson side and the Cargo side
    • The Meson side should be so much better that people really want to use it

In any case, I don't think there is a cause for panic if Meson isn't perfectly interoperable with every build.rs in the world. We have wrapdb, and it works really well. Only a small fraction of crates have really complex build.rs, and I suspect that it would be simpler to maintain a meson.build (or whatever) for those few crates in wrapdb and automatically pull it when pulling packages from crates.io.

@eli-schwartz
Copy link
Member

You're not doing the meson project any favors here.

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.

@eli-schwartz
Copy link
Member

  1. Providing people with very good alternatives to build.rs
    • Both on the Meson side and the Cargo side
    • The Meson side should be so much better that people really want to use it

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.

In any case, I don't think there is a cause for panic if Meson isn't perfectly interoperable with every build.rs in the world. We have wrapdb, and it works really well. Only a small fraction of crates have really complex build.rs, and I suspect that it would be simpler to maintain a meson.build (or whatever) for those few crates in wrapdb and automatically pull it when pulling packages from crates.io.

👍 This was the original plan anyway.

@xclaesse
Copy link
Member Author

In any case, I don't think there is a cause for panic if Meson isn't perfectly interoperable with every build.rs in the world. We have wrapdb, and it works really well. Only a small fraction of crates have really complex build.rs, and I suspect that it would be simpler to maintain a meson.build (or whatever) for those few crates in wrapdb and automatically pull it when pulling packages from crates.io.

That has always been my plan. This PR #11867 allows having meson/meson.build file (either merged upstream, or as wrapdb patch overlay). The generated AST will subdir('meson') if it exists. See 34f7b88.

@dcbaker
Copy link
Member

dcbaker commented Oct 17, 2023

fwiw, I am in favor of doing all of the following:

1. Doing amazing hacks to make `build.rs` work as-is in as many cases as feasible
   
   * Alternatively, ensure that the really-difficult-to-port bits of `build.rs` (like codegen) are supported, and require a different mechanism for the rest (sysdeps, meson.build, etc)

2. Providing people with very good alternatives to `build.rs`
   
   * Both on the Meson side and the Cargo side
   * The Meson side should be so much better that people really want to use it

In any case, I don't think there is a cause for panic if Meson isn't perfectly interoperable with every build.rs in the world. We have wrapdb, and it works really well. Only a small fraction of crates have really complex build.rs, and I suspect that it would be simpler to maintain a meson.build (or whatever) for those few crates in wrapdb and automatically pull it when pulling packages from crates.io.

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

@nirbheek
Copy link
Member

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.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants