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

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

Merged
merged 2 commits into from
Oct 11, 2024

Conversation

xclaesse
Copy link
Member

No description provided.

@xclaesse xclaesse requested a review from jpakkane as a code owner October 10, 2023 15:08
@xclaesse xclaesse added this to the 1.3.0 milestone Oct 10, 2023
@dcbaker
Copy link
Member

dcbaker commented Oct 10, 2023

I think this is a good idea in general, to have an escape hatch if we discover that we've added a mis-feature. I have an alternative idea, which is a bit more standardized and uses the Feature framework: #12358.

@xclaesse
Copy link
Member Author

@dcbaker I removed the mlog.warning() because #12358 is indeed much better. I kept the documentation change because it's not part of your PR. The log_once fix is still good to have, even if now unrelated.

@xclaesse
Copy link
Member Author

@dcbaker alternative you can import 1b25a91 into your PR and then this PR can be postponed to 1.4

@xclaesse xclaesse changed the title cargo: Print a warning telling it's an experimental feature cargo: Document that cargo subprojects is an experimental feature Nov 3, 2023
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'd like to get back to my cleaner solution someday, but I'd rather have this now and replace it with something better later

Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's been "stable" for over a year now so I don't actually see how we can rugpull at this late stage.

@xclaesse
Copy link
Member Author

It's been "stable" for over a year now so I don't actually see how we can rugpull at this late stage.

It's really unfortunate situation, it was never intended to be stable and we have incompatible changes planned. I think in practice current support is too limited to have serious usages of it.

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 11, 2024

I know at least a couple people who are using it because I told them meson supports cargo subprojects.

I know it's annoying to have to maintain both versions but that's what having documented functionality in mature, stable software means.

Even before meson 1.0 we would never outright break functionality that was publicly documented as stable, regardless of how desperate the reasoning was, without giving people a loud warning for a few releases. And we are past 1.0 now.

If it's that necessary to make a hard break and there's no way to support both at the same time, then deprecate cargo subprojects and add truly experimental "cargo2" subprojects that currently do the same thing, but in future versions of meson will branch and do something different instead.

@xclaesse
Copy link
Member Author

To be honest, this is completely your and @dcbaker's fault. I asked many times to merge this PR when it was not too late yet, but you prefered bikeshedding on pointless arguments in #12358.

I'm not interested in keeping compatibility, I definitely don't have time for that. If you want to fork the entire cargo code base be my guest.

@eli-schwartz
Copy link
Member

To be honest, this is completely your and @dcbaker's fault. I asked many times to merge this PR when it was not too late yet, but you prefered bikeshedding on pointless arguments in #12358.

I am very sorry and should have been clearer at the time that I think the entire approach of "don't merge one PR because another PR designed a totally different way to do it" is wrong.

Your PR should always have been merged. I possibly assumed it was obvious and didn't think to mention it. If we look at @dcbaker's initial review of this PR:

I think this is a good idea in general, to have an escape hatch if we discover that we've added a mis-feature. I have an alternative idea, which is a bit more standardized and uses the Feature framework: #12358.

I would argue that this is an approving review of this PR, conditional on the other PR not being ready / having consensus. So when it became clear that the other PR had "bikeshedding on pointless arguments" which I do not believe was bikeshedding or pointless, we should have merged this PR immediately, because it was ready, rather than waiting on a different PR that was not ready.

Instead what happened is we forgot about this PR due to the other PR.

@jpakkane jpakkane modified the milestones: 1.3.0, 1.7 Oct 11, 2024
@xclaesse
Copy link
Member Author

xclaesse commented Oct 11, 2024

Sorry for my ranting, we all do our best and shit happens.

I think forking cargo is unreasonable workload on an already too small team. I strongly think we should just take the risk of breaking the few projects that could be depending on Cargo subproject. We can start warning about that in 1.6.0 and actually break in 1.7.0. We've done breaking changes after a warning cycle in the past, and this is far from a core functionality.

@xclaesse
Copy link
Member Author

Added a release notes snippet to warn about possible future changes.

@jpakkane
Copy link
Member

Given that this is:

  • a module
  • only a warning change
  • not used by that many people (that we know of at least)

I'm going to merge this in for rc2. However if this causes breakages of real world projects then it will be reverted, either in 1.6 or 1.6.1.

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 11, 2024

a module

It is not a module, it is subprojects.

only a warning change

The point of the warning change is to later do a functionality change?

@eli-schwartz
Copy link
Member

eli-schwartz commented Oct 11, 2024

Thinking about this more. I have thought of a potentially significant detail that may change how I think about the entire topic.

Regarding CMake subprojects, we document: https://mesonbuild.com/CMake-module.html

Note: the functionality of this module is governed by Meson's rules on mixing build systems.

Cargo, similar to CMake, involves parsing another build system and translating a meson.build AST out of it.

Cargo, unlike CMake, does not involve running cmake/cargo:

For the purposes of this page, mixing build systems means any and all mechanisms where one build system uses build artifacts from a different build system's build directory in any way.

because we don't run cargo, we just try to reimplement its build description files. According to the strict letter of the "mixing build systems" page, cargo doesn't fall under that rule, but one could argue that what we really mean here is that getting stuff out of another build system is flaky and

The tl/dr version is that while we do provide some functionality for this use case, it only works for simple cases. Anything more complex cannot be made reliable and trying to do that would burden Meson developers with an effectively infinite maintenance burden. Thus these use cases are not guaranteed to work, and even if a project using them works today there are no guarantees that it will work in any future version.

We also say:

On the other hand a "Flatpak-like" approach of building and installing the library with an external mechanism and consuming it via a standard build-system agnostic method such as Pkg-Config would not be considered build system mixing. Use of uninstalled-pkgconfig files is considered mixing, though.

So where does "parse Cargo.toml" fall, here? I genuinely don't know the answer to this question. We should be consistent about it though. I consider it 100% fair game to make the decision that cargo subprojects falls under the "mixing build systems" rule, document this in the "mixing build systems" page, and proceed to make this stability change as well as the linked #12952.

@jpakkane What do you think? Should we read it like that or not?

@jpakkane
Copy link
Member

So where does "parse Cargo.toml" fall, here? I genuinely don't know the answer to this question.

FWICT Cargo considers cargo.toml their own internal format and do not want other people relying on it. It is not a generally usable thing like pkg-config is.

So yes, that rule very much applies here.

@xclaesse xclaesse force-pushed the cargo-experimental branch 2 times, most recently from ee1bca1 to 3e9129c Compare October 11, 2024 20:11
Copy link
Member

@eli-schwartz eli-schwartz left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, I'm happy we were able to find an agreement here (as I didn't like my own suggestion and just felt forced into it).

I'm happy I'm able to retract my request.

docs/markdown/snippets/cargo_exp.md Outdated Show resolved Hide resolved
@xclaesse xclaesse merged commit 304207b into mesonbuild:master Oct 11, 2024
30 of 34 checks passed
@xclaesse xclaesse deleted the cargo-experimental branch October 11, 2024 21:16
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.

4 participants