-
-
Notifications
You must be signed in to change notification settings - Fork 1.8k
Allow specifying method in dependency() and subproject() #15144
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
base: master
Are you sure you want to change the base?
Conversation
It was wasteful, but also failed when the .wrap file miss that dependency from [provide] section and no Cargo.lock file provide it either.
dbc0645
to
228fe3a
Compare
First patch looks good, second should wait for #12057 and a review from other committers. |
If the Cargo subproject is part of the main repo we had to write a dummy wrap file to specify the method: ``` [wrap-file] method=cargo ``` We can now instead directly call `subproject('rust', method: 'cargo')` with no wrap file. This also allows the method to be a user option: `method: get_option('cargo') ? 'cargo' : 'meson'`.
228fe3a
to
40df5d3
Compare
I merged the linked PR. I have another use case for this that came up today, actually. I have a project that has vendored copies of dependencies patched with CMake build support, but I want to use the versions from the wrapdb for the meson support. The result is that Meson errors out with "two subprojects provide the same dependency". Even if this doesn't get us all the way there, it should get us most of the way there. |
Shouldn't we be handling this on a much more fundamental(lying reliable) level by preferring the top level project? And I believe @jpakkane also said this shouldn't be happening anyway because subproject wrap promotion should be an explicit choice by the top level project. :) If we are hitting these sorts of conflicts in the wild then that makes a compelling argument that... @jpakkane is right and we should never have implicitly started recursively loading anything? ... More generally I can understand the desire to avoid adding a wrap file but I don't think it's a very strong reason, and I'm nervous about weakening the existing dependency kwarg. |
The problem I have is that I have a layout like this:
Meson tries to load both of these, and then says "You have two versions of snappy! I quit!" But I can't delete the snappy/ version becuase the CMake build needs it, but I also would rather use a version with native meson build definitions than rely on the CMake dependency fallback. |
@dcbaker your plan is to allow multiple subproject with the same name, but then select the one that supports the We are getting off topic, but I've always wanted to add some config file in e.g. subprojects/wraps.ini where we could have an exclude list to ignore some local directories. |
We do that already of course. The conflict happens only if the same project has 2 wraps that provide the same dep. |
Okay reading the diff now, I see it is a new kwarg that is different between subproject() and dependency(), which I suppose is what I would recommend if I wanted this functionality in some form. I still don't really like this change. The proposed reasoning for wanting this is really cargo specific, imo. And there are good reason for why it's cargo specific -- I'll elaborate on those in a minute. The other argument in favor basically says that this doesn't actually fix anything whatsoever, but indirectly can be used as a kind of excuse to add an unrelated feature, that being use of the method as a backdoor to disambiguate multiple conflicting wrap definitions. We shouldn't have conflicts within a given level and we shouldn't allow subordinate projects to hijack a wrap that the main project, which is fundamentally in charge, decided on. And disambiguating via the method is a really crude way of doing so anyway, because it assumes (blindly) that you won't have the problem of both projects trying to provide a cmake wrap! I have big issues with allowing CMake to be specified here at all. It is conceptually nonsense to specify CMake as the fallback method because CMake does not and cannot do override_dependency anyway. @xclaesse didn't we have this conversation when initially adding wrap method support, already? I view CMake here as effectively dead in the water: you need a wrap file. Even if you didn't need a wrap file, such as for It feels like we've sneakily moved the Overton window on what's considered a good way to interact with subprojects. That makes me wonder if I should have pushed back harder on objecting to adding wrap methods at all, if we will need to continually relitigate these sorts of decisions. Anyway. Cargo wraps. I actually think this is fine, and doesn't need "fallback" methods, because it seems very analogous to how
|
That's a part I don't like, but unfortunately
I've always been against Now you have a good point that cmake subproject does not seems to be using override_dependency(). Maybe it predates it? I don't see why it couldn't do it. Thanks, now I hate that code even more :D |
You still need to tell which API level you want, you can have both foo-1-rs and foo-2-rs. We could in theory drop the -rs suffix when method=cargo, not sure it's worth it. |
We added wrap It is capable today of handling the straightforward cases. There's still cases that aren't straightforward and require the flexibility of the module. And my personal belief is that I am diametrically opposed to permitting a CMakeSubprojectHolder to be returned by
Can I refresh your memory? :) #11730 (comment) It doesn't predate it so much as CMake itself has oppositional defiance disorder with regard to design of build systems, and that specifically conflicts with the kind of guarantees we would like and which dependency overrides require. To wit: dependencies have different names compared to their target names, because dependencies are actually literally just prose documentation about which internal variable to use, the variables will NOT be named the same as either the project or the library or the pkg-config dependency name, and you are simply supposed to not use the other variables at all, even (or especially if) they are internal static libraries that conflict with the name of system dependencies. |
The way I would like to spell it is
This means that you're forced to invoke
|
No, I only want to support the native Meson one because the CMake one doesn't work well for Meson. But the project in question has a CMake build system that will not be removed, and I need to not destroy the CMake build, but make the Meson one work. So what I want is: dependency('snappy', fallback_method : 'meson') I want to never use the CMake one. More like |
This explanation does not compute for me.
but you think the solution to meson seeing a directory named AAA that isn't intended to be parsed as an available wrap, and has a genuine to invoke BBB as Not only don't I understand why this is supposed to be a good idea, I also don't understand how this PR is even supposed to represent a step towards fixing the problem. I am now more convinced than I was before your explanation, that this isn't a good solution to the problem you believe you have. I'd also quite like to know why nobody else has that problem (of having a directory named snappy/ instead of a directory named snappy-1.2.2/) but I'm happy to concede that loading wraps implicitly from their directory name when they are already referenced by a wrap file (???) is a bug regardless of the WrapDB definition for google-snappy.wrap |
Then I've added So right now, there is no way to use the snappy wrap because a subdirectory called snappy exists, which cannot be removed, and which cannot be renamed, and which would be inferior to the warp for my purposes. I absolutely see a keyword that says "Only try a meson subproject and just ignore that cmake thing there" as absolutely solving my problem in the same way the |
That would be fixed by letting a wrap always win over a subdirectory. |
Sure |
#15158 now is in a semi-mergeable state, and implements |
I don't consider it an acceptable solution to this problem, to require magic prompts in The core problem you have is NOT that you want to select which method to use to try to build the subproject. The core problem you have is that it is an error and a fallacy for meson to misdetect one of the two as being a distinct standalone component when it is actually an implementation detail of the other (?). That is very different from a system pkg-config file and a system foo-config.cmake that each exist independently and don't relate to each other at all, and may have unique pros and cons. Hence @bonzini's suggestion was also my belief: we shouldn't be loading a directory as an implicit wrap, if an explicit Adding new meson.build syntax instead, would be totally backwards. So if this was the only reason to add a new syntax, I would 100% object to the new syntax. And if there are other reasons to add a new syntax (and this PR was in fact about a totally different reason to add a new syntax) I want to exclusively discuss that other reason, rather than this reason that seems objectively bad. |
If the Cargo subproject is part of the main repo we had to write a dummy
wrap file to specify the method:
We can now instead directly call
subproject('rust', method: 'cargo')
with no wrap file. This also allows the method to be a user
option:
method: get_option('cargo') ? 'cargo' : 'meson'
.