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

Support for disabling optional dependencies even if they can be found. #8224

Closed
Neumann-A opened this issue Jan 20, 2021 · 21 comments
Closed

Comments

@Neumann-A
Copy link
Contributor

Neumann-A commented Jan 20, 2021

more precise description in: #8224 (comment)


continuation of #2452
Seems like this is not solved. #2411 was closed and not merged.
Even with #3376 it actually requires meson.build to do the correct thing (which it often does not or the build script is a mix of correct and wrong).

You probably want to have a look at CMAKE_DISABLE_FIND_PACKAGE_<PackageName> from CMake to actually learn from it.
On a different but related issue: I also saw people suggesting/looking for something like <PackageName>_DIR from CMake

@eli-schwartz
Copy link
Member

<package>_DIR is the directory containing the foo-config.cmake file. meson simply uses pkg-config, which is a standard and comes with PKG_CONFIG_PATH.

CMAKE_DISABLE_FIND_PACKAGE_<package> is a terrible, terrible, terrible UI because it does spooky action at a distance and what you actually want is to disable a feature, not the dependency the feature depends on, because otherwise you have no clue why it's being disabled.

"But people don't implement the ability to choose in their meson.build, therefore let's add new core meson features" is not really a convincing argument IMO.

FWIW, I was initially introduced to the fact that this cmake "feature" exists, by someone who was calmly explaining to me why "our project does not need options, see, you could just use CMAKE_DISABLE_FIND_PACKAGE for everything if you know which dependencies are internally used by the features you don't want".

I took a hard pass on that one. :(

@Neumann-A
Copy link
Contributor Author

_DIR is the directory containing the foo-config.cmake file. meson simply uses pkg-config, which is a standard and comes with PKG_CONFIG_PATH.

Yeah but what if there is something in there I don't want meson to pick up.
I am mainly interested in disabling optional deps because enabling by rerouting can be done via PKG_CONFIG_PATH as you said.

CMAKE_DISABLE_FIND_PACKAGE_ is a terrible, terrible, terrible UI because it does spooky action at a distance

From experience it is the only effective one to disable package lookup completely. It is not spooky. Spooky is all the optional dependencies getting picked up by "build order" in vcpkg due to not being able to properly disable those deps or adding them as required dependencies to "correct" the build order (although they are optional).

"But people don't implement the ability to choose in their meson.build, therefore let's add new core meson features" is not really a convincing argument IMO.

It is. In the end user inability needs to be taken into account.

CMAKE_DISABLE_FIND_PACKAGE for everything if you know which dependencies are internally used by the features you don't want".

Read the docs: https://cmake.org/cmake/help/latest/module/FeatureSummary.html.

In the end it doesn't matter how good a build system claims to be if the users writing the build scripts don't know how to do it properly. On the other hand the people packaging such libraries need escape hatches to not patch the hell out of every stupid build script they encounter (and no I don't have the time to correct every build script and submit the required changes upstream.).

@eli-schwartz
Copy link
Member

eli-schwartz commented Jan 20, 2021

Yeah but what if there is something in there I don't want meson to pick up.

Them logically you don't want to tell meson where it can be found, you want to tell meson not to find it.

I am mainly interested in disabling optional deps

... of course you are. So DIR would be a duplicate, unneeded functionality if DISABLE_FIND_PACKAGE exists.

getting picked up by "build order" in vcpkg due to not being able to properly disable those deps or adding them as required dependencies to "correct" the build order (although they are optional).

The root of the problem is people deliberately lying in their build descriptions. And if they're marking it "required" then meson is going to darn well respect that, so DISABLE_FIND_PACKAGE would lead to a configure time error due to missing required dependencies you manually told meson should be set as missing.

It is. In the end user inability needs to be taken into account.

No, mildly tweaking the wording in order to say the same thing is still not a convincing argument. It's a convincing explanation of why you decided you want it, no more, no less.

And it's still a project bug which is refusing to give the user an ability. Ask for feature options to toggle optional features.

Read the docs: https://cmake.org/cmake/help/latest/module/FeatureSummary.html.

That requires the project to both agree to support printing options/packages (and annotate them extensively) and not care about doing it properly with options.

It also requires you to do a full configure cycle, get it wrong, then figure out what to do next time.

@eli-schwartz
Copy link
Member

I have actually, for real, told people "cmake sucks, don't use cmake. Cmake lets you do nonsense like DISABLE_FIND_PACKAGE instead of proper options. Use meson instead."

It would feel pretty weird to me if meson then went around and added support for this misfeature.

@Neumann-A
Copy link
Contributor Author

Them logically you don't want to tell meson where it can be found, you want to tell meson not to find it.

All packages are installed into the same installed dir so not setting the PKG_CONFIG_PATH would result in meson not finding any dependency so that is not an option.

... of course you are. So DIR would be a duplicate, unneeded functionality if DISABLE_FIND_PACKAGE exists.

You did not read my issue well enough. "On a different but related issue:". So no discussion about _DIR vars required.

The root of the problem is people deliberately lying in their build descriptions. And if they're marking it "required" then meson is going to darn well respect that, so DISABLE_FIND_PACKAGE would lead to a configure time error due to missing required dependencies you manually told meson should be set as missing.

disabling required packages is in general not possible and leads to an expected error. This is also the case in CMake.

That requires the project to both agree to support printing options/packages (and annotate them extensively) and not care about doing it properly with options.

no setup required. It will just not print a description in that case.

It also requires you to do a full configure cycle, get it wrong, then figure out what to do next time.

it is actually the same for meson in my case. (And it is actually the only correct way to do it.)

I have actually, for real, told people "cmake sucks, don't use cmake. Cmake lets you do nonsense like DISABLE_FIND_PACKAGE instead of proper options. Use meson instead."

It is not nonsense it is just your inability to understand and use it correctly. People will always write bad build scripts no matter which buildsystem is used. I have seen my fair share of broken buildscripts in cmake/meson/autotools/qmake/plain make. It doesn't matter in the end.

@mensinda
Copy link
Member

So, if I understand correctly, the main issue here is that some projects do stuff like:

someDep = dependency('someDep', required: false)

if someDep.found()
  # Do stuff you want to explicitly disable even if someDep is found
  # and the user did not provide another toggle for this condition
endif

If this is the case, I would suggest trying to find a solution for this particular use case, instead of arguing why/if exactly, CMAKE_DISABLE_FIND_PACKAGE is bad. Also, we could argue if we want to actually do something about this / if we actually recognize this as a problem. Debating some CMake design decisions won't be to anyone's benefit.

@eli-schwartz
Copy link
Member

no setup required. It will just not print a description in that case.

I said "in order to achieve value out of FeatureSummary, you need to do setup". Your response is "you don't need to do setup if you don't use it at all"?

I'm confused by your reply.

It also requires you to do a full configure cycle, get it wrong, then figure out what to do next time.

it is actually the same for meson in my case. (And it is actually the only correct way to do it.)

You might not have been aware, but it is NOT the same for meson. meson configure sourcedir/ prints all available options without configuring. It is very much the correct way to do it. ./configure --help for autotools works the same way, no need to run a full configure just to inspect available options.

cmake does not support this. cmake only supports printing fully configured statuses after running the full configure cycle.

I blame this on the fact that cmake does not HAVE options. Only internal state variables (used in turing-complete user scripting performing common snippets of library probing) within the cmake AST, which you can define on the command line.

It is not nonsense it is just your inability to understand and use it correctly. People will always write bad build scripts no matter which buildsystem is used. I have seen my fair share of broken buildscripts in cmake/meson/autotools/qmake/plain make. It doesn't matter in the end.

It is my inability to philosophically view it as a good thing. Please don't tell me I don't "understand" it -- I understand it just fine.

@mensinda
Copy link
Member

I can think of two solutions to fix this:

  • implementing CMAKE_DISABLE_FIND_PACKAGE as options (highly unlikely) or in cross/native files (not ideal but more likely)
  • Making use of meson.override_dependency to return a not found dependency and making this more accessible.

You should be, theoretically, already be able to do what you want with override_dependency and a not found dependency or by creating a super project including the project you want to build as a subproject.

project('root', ['c'])

meson.override_dependency('zlib', dependency('erferfergwrrgbwrbwr', required: false))

subproject('p1')
project('p1', ['c'])

dep = dependency('zlib', required: false)

if dep.found()
  message('P1: FOUND')
else
  message('P1: NOT FOUND')
endif

This is obviously not an ideal solution, but it shows that meson can theoretically already do this.

Also, can we please stop arguing about what CMake does, whether it is correct, arguing about semantics, and focus on actually fixing the issue by providing a solution.

@eli-schwartz
Copy link
Member

eli-schwartz commented Jan 20, 2021

If this is the case, I would suggest trying to find a solution for this particular use case, instead of arguing why/if exactly, CMAKE_DISABLE_FIND_PACKAGE is bad. Also, we could argue if we want to actually do something about this / if we actually recognize this as a problem. Debating some CMake design decisions won't be to anyone's benefit.

Given the actual ticket we are in explicitly says to use CMAKE_DISABLE_FIND_PACKAGE for inspiration on what cmake is doing that the OP wishes meson did, I think it's pretty relevant why it's bad. It's bad because it doesn't solve the problem by creating a feature option.

And, my entire argument is "here are the reasons why we don't want to do something about it, because it's bad when cmake does it and it will be bad if meson does it too".

There are two different ways to solve this in meson today:

  • submit a project bug report "do not do stuff like this"
  • decide you don't have time for that, you need to override it for your own use but not contribute upstream; in this case, sed meson.build from dependency('foo', required: false) to dependency('', required: false)

@Neumann-A
Copy link
Contributor Author

someDep = dependency('someDep', required: false)

if someDep.found()
  # Do stuff you want to explicitly disable even if someDep is found
  # and the user did not provide another toggle for this condition
endif

@mensinda exactly

unfortunately some meson.builds try very hard in finding optional deps e.g. (as seen in gtk 4.0.1):

someDep = dependency('someDep', required: false)
if not someDep.found()
  someDep = cc.find_library('someDep' <or whatever arguments are required>)
endif
if someDep.found()
  # Do stuff you want to explicitly disable even if someDep is found
  # and the user did not provide another toggle for this condition
endif

so I need a way to deactivate the dependency() call and the find_library(). CMake offers a way to deactivate both without ever touching the build scripts.
I don't mind the deactivation in native/cross files since this is easily added to the native/cross files generated within vcpkg to run the build. Simply add a [override_dependency] section to it? (although this doesn't take care of the find_library call.)

@eli-schwartz hate on CMake all you like.... due to your response I cannot take you seriously any more and will simply ignore you.
The correct call for dependency would probably have been something like : dependency('someDep', linked_option: '<someoption>') but that ship sailed already.

@mensinda
Copy link
Member

If I understand the use case here correctly, it is for integrating meson projects with vcpkg (or any other packaging system) and not about one or two external meson projects you want to build by hand. In this case, I would argue that submitting patches upstream is unreasonable to ask (while I agree that this would be the ideal solution of course). Also, patching the meson.build files also does not speak for the qualities of meson. I would argue that having to read, understand and modify meson.build files to get the desired result is a bad user experience.

Given the actual ticket we are in explicitly says to use CMAKE_DISABLE_FIND_PACKAGE for inspiration on what cmake is doing that the OP wishes meson did, I think it's pretty relevant why it's bad. It's bad because it doesn't solve the problem by creating a feature option.

OK, fair point. As I said previously I agree that implementing feature options is the ideal way to go. My point is to rather focus on the problem I described in my first comment and trying to find any solution for this since this is the actual issue here and not implementing CMAKE_DISABLE_FIND_PACKAGE. This way we can also try to find different solutions, be they short term or long term.

Not doing this is essentially equivalent to telling packaging maintainers: We don't want to deal with this problem, either go ask 100+ repos to fix their stuff or invest a lot of time to patch their builds.

We can of course go with this message as a project, but this would essentially be us admitting that there is a real-world problem (users not doing dependencies correctly by not using the tools we provide) which is likely (at least partially) because of our design decisions.

FWIW, I was initially introduced to the fact that this cmake "feature" exists, by someone who was calmly explaining to me why "our project does not need options, see, you could just use CMAKE_DISABLE_FIND_PACKAGE for everything if you know which dependencies are internally used by the features you don't want".

I 100% agree that we should avoid this scenario at all costs.


so I need a way to deactivate the dependency() call and the find_library()

Uff, also deactivating find_library() would be a hard sell, even for me. I would assume/hope that these cases would be at least semi-rare so patching upstream could be feasible. We currently only provide overrides for dependency and find_program. Also essentially adding overrides to cc.find_library() will be hard to justify since this should essentially just be the equivalent of directly asking the compiler. Adding additional logic here would be very surprising in a lot of ways...

Simply add a [override_dependency] section to it?

I am not exactly sure how to handle this, because while my workaround works, it is arguably abusing the override_dependency function. By adding an override_dependency we would not only have to support disabling dependencies and a way to declaring dependencies. I am also not 100% sure if this is a good idea since there would be some serious implications...

due to your response I cannot take you seriously any more and will simply ignore you.

I know that this is a heated discussion, but can we please stop with stuff like this?


Another question would be: Do we really need required: false anymore if we have features? As in can we safely deprecate it and force users to use our semi new feature options? This way this issue would solve itself over time, we won't need to implement something like CMAKE_DISABLE_FIND_PACKAGE and would arguably be a better design overall.

I am fully aware that this change would take years to have any effect and offers no short term solution.

@mensinda
Copy link
Member

Also, @Neumann-A could you please update your first comment (and maybe also the title of this issue) with a better description of this issue so other people have a better understanding of what it actually is you want to solve here?

@Neumann-A Neumann-A changed the title Support explicitly enabling/disabling optional dependencies via a cmdline option? Support for disabling optional dependencies even if they can be found. Jan 20, 2021
@Neumann-A
Copy link
Contributor Author

@mensinda Better? Otherwise supply me with a text I should add ;)

Uff, also deactivating find_library() would be a hard sell, even for me

In CMake I consider raw find_library calls outside a FindModule a code smell.
I don't know if meson has something similar to cmake's FindModules.

@mensinda
Copy link
Member

@mensinda Better? Otherwise supply me with a text I should add ;)

Yes, thanks.

In CMake I consider raw find_library calls outside a FindModule a code smell.
I don't know if meson has something similar to cmake's FindModules.

Meson does not have modules. Using the dependency function is usually recommended since it also takes care of include directories, version, etc. find_library is usually only used for libraries without *.pc and *.cmake files (and there is no built in meson dependency that handles libraries which are an absolute pain like boost).

A common use case is cc.find_lirary('dl'). This is usually not a code smell but cc.find_lirary('boost_filesystem-mt-x86-64') would be.

@Neumann-A
Copy link
Contributor Author

Neumann-A commented Jan 20, 2021

Meson does not have modules.

it does you are just not aware of it. dependecy('whatever') is the equivalent to find_package(whatever). The fact that you have buildin deps already prove that. The question is if users can extend on these builtin dependencies or not. CMake as CMAKE_MODULE_PATH to extend the builtin modules. The question is thus does meson have something similar to extend the builtin deps by the user?

A common use case is cc.find_lirary('dl')

  1. Why would you ever need to ask this question? (and not have an option for it with_dl)
  2. wouldn't it be better as builtin dep: dependency('dl') or even have it as a general system/compiler introspection

@mensinda
Copy link
Member

it does you are just not aware of it. dependecy('whatever') is the equivalent to find_package(whatever).

I guess this depends on the definition of module. We don't have modules in the way that there is no way to include custom meson code that does arbitrary (and potentially stupid) stuff to look up a dependency. We try our best to encourage the use of *.pc files since they are not turning complete, easy to read and write, and actually designed to define how a dependency should be used.

For your last to points:

Ideally yes, but we would have to maintain a lot of additional code in this case if we want to support all/most common libraries like this. And even worse: We would most likely just end up calling cc.find_library from python instead of meson which solves basically nothing. We also can't remove find_library since some projects do have a legitimate use for it and we can't put everything into system dependencies. We explicitly also don't want another FindXYZ.meson eco-system that ends up copy-pasted around. And finally, cc.find_library('dl') works well enough as a common case.

@Neumann-A
Copy link
Contributor Author

We don't have modules in the way that there is no way to include custom meson code that does arbitrary (and potentially stupid) stuff to look up a dependency.

Yeah your way seems to be to define a superproject to do the "extra" stuff (#8224 (comment)) ;)
I certainly can come up with an automatic way to create the superproject from cmake but do i really want to do that?

We try our best to encourage the use of *.pc files since they are not turning complete, easy to read and write

*.pc files are nice but also potentially broken. vcpkg fixes that by not requiring --static for pkg-config if the library was built as a static library. It is basically a design flaw to encoded static&shared into the same file. Furthermore *.pc files sometimes are incorrect. Even meson does not generate completely correct *.pc files if find_library and or dependency with a config.cmake are used.

We also can't remove find_library since some projects do have a legitimate use for it

Which is symbol lookup (e.g. AC_SEARCH_LIBS)? I would say something like this and the attached logic should always be encapsulated outside a meson.build file into some sort of dependency('') call.

We explicitly also don't want another FindXYZ.meson eco-system that ends up copy-pasted around

Mark my words: It will happen sooner or later..... if your internal dependency lookup is unstable users want that. You are otherwise locking in users to a certain version of meson, especially since you don't fear breaking existing meson.build files. (and this is only one reason they exist in CMake)

So enough time invested in meson, I know return to the things I care more about ;)

@eli-schwartz
Copy link
Member

If I understand the use case here correctly, it is for integrating meson projects with vcpkg (or any other packaging system) and not about one or two external meson projects you want to build by hand. In this case, I would argue that submitting patches upstream is unreasonable to ask (while I agree that this would be the ideal solution of course).

Why is it different to build one project in vcpkg vs. building the same meson.build by hand? Surely it does the same dependency lookups, reads the same command-line options, and respects the same --native-file functionality...

Is there ever a time when it's "unreasonable" to submit patches upstream? Why would these patches not be generically useful?

Also, patching the meson.build files also does not speak for the qualities of meson. I would argue that having to read, understand and modify meson.build files to get the desired result is a bad user experience.

It doesn't speak for the qualities of meson, either positively or negatively. It does speak (negatively) for the quality of one specific meson.build that is "doing the wrong stuff".

I specifically suggested "or you could sed the meson.build" as a response to "I know I want to disable this specific dependency lookup but I have no time to submit an upstream project bug report requesting a feature option".

Not doing this is essentially equivalent to telling packaging maintainers: We don't want to deal with this problem, either go ask 100+ repos to fix their stuff or invest a lot of time to patch their builds.

We can of course go with this message as a project, but this would essentially be us admitting that there is a real-world problem (users not doing dependencies correctly by not using the tools we provide) which is likely (at least partially) because of our design decisions.

I was with you here all the way through real-world problems, telling package maintainers to invest time in patching things, etc... until you got to "which is likely at least partly due to our design decisions".

Can you please explain this? How is "users not doing dependencies correctly by not using the tools we provide" (read: users refusing to use meson_options.txt and get_option to implement configure-time choices) fallout from meson design decisions? What design decision are you blaming here?

I 100% agree that we should avoid this scenario at all costs.

Fortunately, I kind of avoided that scenario too. Even though the person I referenced, directly NACKed adding cmake -DOPTION variables to control enabling features ("they're inelegant, DISABLE_FIND_PACKAGE is the best"), someone else went and added -DOPTION variables for all features and was able to get it merged despite lack of support from... certain sides.

It's still something I would like to make actually 100% impossible though. :p

Another question would be: Do we really need required: false anymore if we have features? As in can we safely deprecate it and force users to use our semi new feature options? This way this issue would solve itself over time, we won't need to implement something like CMAKE_DISABLE_FIND_PACKAGE and would arguably be a better design overall.

We definitely need it. Example from one of my projects

gpgme = dependency('gpgme',
                   required : false,
                   static : get_option('buildstatic'))
# gpgme recently began providing a pkg-config file. Create a fake dependency
# object if it cannot be found, by manually searching for libs.
if not want_gpgme.disabled() and not gpgme.found()
  gpgme_config = find_program('gpgme-config', required : want_gpgme)
  if gpgme_config.found()
    gpgme_version = run_command(gpgme_config, '--version').stdout().strip()

    needed_gpgme_version = '>=1.3.0'
    if gpgme_version.version_compare(needed_gpgme_version)
      gpgme_libs = [
        cc.find_library('gpgme',
                        dirs : [get_option('gpgme-libdir')]),
        cc.find_library('gpg-error',
                        dirs : [get_option('gpgme-libdir')]),
        cc.find_library('assuan',
                        dirs : [get_option('gpgme-libdir')]),
      ]
      gpgme = declare_dependency(dependencies : gpgme_libs)
    endif
  endif
endif

Or one might look for dependency('python3-embed', required: false) and fall back to dependency('python3') for older versions of python.

Both of these cherry-picked examples are fixed in later versions of meson by special-casing config-tool handling or import('python3').find_installation() with the embed keyword. But users need this flexibility in the general case (plus why use the python module just for this).

@mensinda
Copy link
Member

Surely it does the same dependency lookups, reads the same command-line options, and respects the same --native-file functionality...

Is there ever a time when it's "unreasonable" to submit patches upstream? Why would these patches not be generically useful?

I was more referring to the amount of work involved for doing so compared to manually building a few projects. It is practically unreasonable to ask package maintainers to submit patches for every project upstream due to the amount of work involved. Of course, submitting patches upstream would be the ideal solution, I didn't try to imply otherwise.

Can you please explain this?

I was trying to say that we should strive to make it impossible (or at least very hard) for users to make mistakes. Especially since it is currently just more convenient to not bother about adding options since there is more work involved.

We definitely need it. Example from one of my projects

Fair point, I was hoping that #4595 might help here (in 99% of all cases)...

@Neumann-A
Copy link
Contributor Author

Fair point, I was hoping that #4595 might help here (in 99% of all cases)...

ah yes..... that nonsense looks exactly like the things Qt5 does.
the lookup rules are basically always incomplete and lacking.
The important thing is to give the user a way to explicitly define the whatever_dep var outside of the meson.build

@eli-schwartz
Copy link
Member

Nothing to do here. Meson provides feature options for this.

@eli-schwartz eli-schwartz closed this as not planned Won't fix, can't repro, duplicate, stale May 16, 2024
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

No branches or pull requests

3 participants