cargo: Fix feature resolution #12952

xclaesse · 2024-03-10T01:01:38Z

Introduce a global Cargo interpreter state that keeps track of enabled
features on each crate.

Before generating AST of a Cargo subproject, it downloads every
sub-subproject and resolves the set of features enabled on each of them
recursively. When it later generates AST for one its dependencies, its
set of features and dependencies is already determined.

This is based on top of #12945

run_unittests.py

mesonbuild/cargo/__init__.py

swick · 2024-06-17T15:33:27Z

Before this MR it was possible to have a wrap or Cargo.lock for one specific crate which then also has a Cargo.lock with its own dependencies. With this MR, that doesn't seem to be the case anymore.

xclaesse · 2024-06-17T15:51:39Z

@swick I'm not sure exactly what you mean. If the main project has a wrap for a cargo dependency, the Cargo.lock from that dep will be used as well.

The big limitation this PR leaves is when you have multiple "entry point" into cargo. For example when the main project does :

adep = dependency('a-rs')
bdep = dependency('b-rs')

If a and b have a common c-rs crate, it will be configured with whatever features a needs. If b then needs extra features that's an error.

The easy workaround for that would be to have a single entry point, for example by adding rust.dependencies('a-rs', 'b-rs').

xclaesse · 2024-06-17T15:58:24Z

@swick I'm not sure exactly what you mean. If the main project has a wrap for a cargo dependency, the Cargo.lock from that dep will be used as well.

Actually, reading the code again (been a while), it's a bit more complicated than that. The way Cargo works is the top level Cargo.lock takes precedence over all other dependencies. The main project's Cargo.lock is supposed to contain the whole dependency tree. It may not work exactly that way in Meson currently... need to double check.

xclaesse · 2024-06-17T17:47:16Z

@swick I think I understood what you mean and I added a commit to fix your case. Can you test it again please?

swick · 2024-06-17T19:39:46Z

Same error locally as in CI.

e: self.is_subproject is a function: if not self.is_subproject():

swick · 2024-06-17T20:13:56Z

With that fixed it's behaving as before.

e: The questions are answered in the docs update of this PR. I still don't know how to make my setup work though.

If I have tow dependencies, their features are not unified though, are they? This is what I currently get:

subprojects/serde_derive-1.0.102/meson.build:67:-1: ERROR: Problem encountered: Dependency syn-1-rs previously configured with features ['quote', 'default', 'clone-impls', 'derive', 'parsing', 'printing', 'proc-macro'] but need ['quote', 'visit', 'default', 'clone-impls', 'derive', 'parsing', 'printing', 'proc-macro']

I'm also failing to turn on the specific feature:

project(...
  default_options: {
    'syn-1-rs:feature-visit': true,
  },
)

subprojects/syn-1.0.82/meson.build:1:-1: ERROR: In subproject syn-1-rs: Unknown options: "syn-1-rs:feature-visit"

xclaesse · 2024-06-17T22:03:33Z

If I have tow dependencies, their features are not unified though, are they?

They are not unified, that's the limitation I mentioned that could be worked around by adding e.g. rust.dependencies('a-rs', 'b-rs').

I'm also failing to turn on the specific feature:

This PR removes those per subproject feature options because features are global. I think we should add a global module option like -Drust.features=foo,bar. More similar to cargo: https://doc.rust-lang.org/cargo/reference/features.html#command-line-feature-options.

As workaround for now, you can create a dummy cargo project that depends on both crates you need, and use that as meson subproject.

swick · 2024-06-17T23:30:30Z

As workaround for now, you can create a dummy cargo project that depends on both crates you need, and use that as meson subproject.

Yeah, that seems to work. Now the biggest issue seems to be target.cfg.dependencies not working.

eli-schwartz · 2024-10-08T01:36:16Z

docs/markdown/snippets/cargo_features.md

+Breaks: This change removes per feature Meson option That were previously
+possible to set as show above or from command line `-Dfoo-rs:feature-foo=true`.


Wouldn't this also break projects that are setting those options via default_options: [...]?

Yes that's an incompatible change for cargo subprojects, but we never claimed that was stable. Hopefully not many projects actually uses it yet.

I'm looking and I cannot find a single place where we claim it is not stable, either as a meson warning (the way we print for import() of an unstable module) or in the documentation.

I think the technical term for this is "we blew it".

It's here: #12358. I asked many times to merge that before release...

I think the technical term for this is "we blew it".

yes... :/

Actually my PR for that was #12356 but then @dcbaker wanted to make a more complex but better solution, and we lost track if it... :/

Yeah, we did. I meant to just merge 12356 and replace it later if I ever got 12358 working. That's on me

Introduce a global Cargo interpreter state that keeps track of enabled features on each crate. Before generating AST of a Cargo subproject, it downloads every sub-subproject and resolves the set of features enabled on each of them recursively. When it later generates AST for one its dependencies, its set of features and dependencies is already determined.

The library name defaults to its package name, but it can be different. For example: - package name: cairo-sys-rs - library name: cairo-sys - dependency name: ffi

In the case the main project has a .wrap file for a cargo subproject, that subproject's Cargo.lock must be loaded before we can recursively fetch all its dependencies.

xclaesse requested a review from jpakkane as a code owner March 10, 2024 01:01

github-advanced-security bot found potential problems Mar 10, 2024

View reviewed changes

run_unittests.py Dismissed Show dismissed Hide dismissed

mesonbuild/cargo/__init__.py Fixed Show fixed Hide fixed

xclaesse force-pushed the cargo-global-state branch 3 times, most recently from 0ca3969 to 8008bb7 Compare March 11, 2024 13:45

eli-schwartz added the language:rust label Mar 27, 2024

xclaesse force-pushed the cargo-global-state branch 2 times, most recently from d2da16b to 4aa5b35 Compare June 17, 2024 15:16

xclaesse force-pushed the cargo-global-state branch from 9cfcdd8 to 856ef89 Compare June 17, 2024 21:57

xclaesse force-pushed the cargo-global-state branch from 856ef89 to 9e60bec Compare June 17, 2024 22:11

xclaesse force-pushed the cargo-global-state branch from 9e60bec to 93534fd Compare July 17, 2024 15:56

xclaesse force-pushed the cargo-global-state branch from 93534fd to e527d8a Compare October 7, 2024 21:32

eli-schwartz requested changes Oct 8, 2024

View reviewed changes

This was referenced Oct 11, 2024

cargo: Add support for system-deps dependencies #13778

Merged

cargo: Run build.rs to get extra --cfg args #13779

Open

eli-schwartz mentioned this pull request Oct 11, 2024

cargo: Document that cargo subprojects is an experimental feature #12356

Merged

xclaesse added 3 commits October 11, 2024 17:18

cargo: Use the library name instead of package name

27db7c1

The library name defaults to its package name, but it can be different. For example: - package name: cairo-sys-rs - library name: cairo-sys - dependency name: ffi

cargo: Load Cargo.lock and subprojects/*.wrap while recursing

1070e56

In the case the main project has a .wrap file for a cargo subproject, that subproject's Cargo.lock must be loaded before we can recursively fetch all its dependencies.

xclaesse force-pushed the cargo-global-state branch from e527d8a to 1070e56 Compare October 11, 2024 21:18

xclaesse closed this Oct 24, 2024

xclaesse deleted the cargo-global-state branch October 24, 2024 15:00

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

cargo: Fix feature resolution #12952

cargo: Fix feature resolution #12952

xclaesse commented Mar 10, 2024

swick commented Jun 17, 2024

xclaesse commented Jun 17, 2024

xclaesse commented Jun 17, 2024

xclaesse commented Jun 17, 2024

swick commented Jun 17, 2024 •

edited

Loading

swick commented Jun 17, 2024 •

edited

Loading

xclaesse commented Jun 17, 2024

swick commented Jun 17, 2024

eli-schwartz Oct 8, 2024

xclaesse Oct 9, 2024

eli-schwartz Oct 9, 2024

xclaesse Oct 9, 2024

xclaesse Oct 9, 2024

xclaesse Oct 9, 2024

eli-schwartz Oct 9, 2024

dcbaker Oct 9, 2024

		Breaks: This change removes per feature Meson option That were previously
		possible to set as show above or from command line `-Dfoo-rs:feature-foo=true`.

cargo: Fix feature resolution #12952

cargo: Fix feature resolution #12952

Conversation

xclaesse commented Mar 10, 2024

swick commented Jun 17, 2024

xclaesse commented Jun 17, 2024

xclaesse commented Jun 17, 2024

xclaesse commented Jun 17, 2024

swick commented Jun 17, 2024 • edited Loading

swick commented Jun 17, 2024 • edited Loading

xclaesse commented Jun 17, 2024

swick commented Jun 17, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

swick commented Jun 17, 2024 •

edited

Loading

swick commented Jun 17, 2024 •

edited

Loading