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

Ignore cmake subprojects on normal fallbacks #14007

Closed
wants to merge 1 commit into from

Conversation

Jan200101
Copy link
Contributor

the normal dependency() function doesn't support cmake defines or various overrides that may be desired for compatibility so its best to completely ignore cmake subprojects unless explicitly fetched by cmake.subproject()

message and comment could probably be improved

the normal dependency() function doesn't support cmake defines or various overrides that may be desired for compatibility so its best to completely ignore cmake subprojects unless explicitly fetched by cmake.subproject()
@Jan200101 Jan200101 requested a review from jpakkane as a code owner December 14, 2024 19:28
@Jan200101
Copy link
Contributor Author

It seems there is a test case for this exact logic
https://github.com/mesonbuild/meson/blob/e542901af6e30865715d3c3c18f703910a096ec0/test%20cases/cmake/27%20dependency%20fallback/meson.build

I'm of the opinion that resolving cmake subprojects from a normal dependency() call is wrong

@eli-schwartz
Copy link
Member

the normal dependency() function doesn't support cmake defines or various overrides that may be desired for compatibility so its best to completely ignore cmake subprojects unless explicitly fetched by cmake.subproject()

The best way to completely ignore cmake subprojects for compatibility is to not have them in your project tree. Why would you add one if you didn't want to use it?

Note that you can pass dependency('cmakeproj', cmake_args: ['-DENABLE_FOO=ON']).

So it's my impression that the motivation for this PR was essentially just a documentation issue -- the cmake_args kwarg was hard to discover but seemingly would have been an acceptable solution?

Unfortunately there's still a difference between meson subprojects and cmake subprojects inasmuch as you cannot override options from the latter via command-line arguments to meson setup ...

@eli-schwartz
Copy link
Member

Looking further the cmake_args support appears to be mildly bonkers and only gets used for dependencies with method: 'cmake', not subprojects. Lame...

It is also not tested in CI at all.

Still I'd rather make this use case work somehow, rather than delete functionality. Also, if you want to delete this functionality, the correct way to do it is to fully revert #11730 rather than add a check that renders some code branches nonfunctional.

@Jan200101
Copy link
Contributor Author

The best way to completely ignore cmake subprojects for compatibility is to not have them in your project tree. Why would you add one if you didn't want to use it?

I want the subproject to be used but I also want to configure it so I don't have 80 different features I don't use embedded in my binary

Note that you can pass dependency('cmakeproj', cmake_args: ['-DENABLE_FOO=ON']).

As mentioned by you (and be me on Matrix) cmake_args is not supported by cmake fallbacks
it only checks for cmake_options and options which are suppose to be set by cmake.subproject()

Unfortunately there's still a difference between meson subprojects and cmake subprojects inasmuch as you cannot override options from the latter via command-line arguments to meson setup ...

there is a larger difference in how meson and cmake work which make automatic cmake fallbacks unsuited.

Still I'd rather make this use case work somehow, rather than delete functionality. Also, if you want to delete this functionality, the correct way to do it is to fully revert #11730 rather than add a check that renders some code branches nonfunctional.

reverting 11730, even just one commit, would cause a regression because now established functionality would suddenly stop working, this is absolutely NOT an option.

this PR doesn't delete functionality either, it explicitly makes it so that cmake subprojects are only resolved when they can be explicitly handled e.g. by cmake.subproject()

I'm open to adapting this, however I don't see a way of incorporating the existing cmake subproject system into dependency() without implicitly removing any support for other CMakeSubprojectOptions functionality.

support for cmake subproject should be handled in the cmake module only, it doesn't make sense to do this elsewhere.

@eli-schwartz
Copy link
Member

reverting 11730, even just one commit, would cause a regression because now established functionality would suddenly stop working, this is absolutely NOT an option.

this PR doesn't delete functionality either, it explicitly makes it so that cmake subprojects are only resolved when they can be explicitly handled e.g. by cmake.subproject()

No, what I said is that this PR does revert 11730 today. The github checks that are failing are checking to see if 11730's added feature works, namely:

  • create a foo.wrap which has a [provide] section
  • add method=cmake to that wrap
  • use dependency('foo')

Mechanically, your PR requires that a wrap file containing method=cmake is only configured using cmake if it is also invoked via a python do_subproject(...) that sets force_method='cmake', something exclusively done via the call in mesonbuild/modules/cmake.py

So, merging this PR causes the established functionality (of respecting method=cmake in a .wrap file) to suddenly stop working. If that's the goal, then I said that reverting 11730 using git revert would be a code cleanup whereas the current state of this PR does effectively the same thing but with unreachable branches instead.

Reverting 11730 would also remove the test cases which are currently failing in this PR.

@eli-schwartz
Copy link
Member

It's (still) not clear to me how this scenario comes up.

The original claim by @xclaesse when he implemented #11730 was that normal fallbacks should never use a cmake subproject unless the wrap file contains method=cmake to explicitly indicate that it's okay to parse it as a dependency fallback using cmake.

A regular cmake subproject (does not contain method= in the wrap file, and is expected to be manually invoked using cmake.subproject() including passing CMakeSubprojectOptions) shouldn't be getting used by dependency() at all, so if it is then maybe that is a bug instead.

Is there a testcase demonstrating the problematic fallback you are trying to avoid?

@Jan200101
Copy link
Contributor Author

closing because I'm no longer interested in working on this

@Jan200101 Jan200101 closed this Jan 28, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants