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

Document or improve empty dependency objects #2324

Closed
jon-turney opened this issue Sep 15, 2017 · 9 comments
Closed

Document or improve empty dependency objects #2324

jon-turney opened this issue Sep 15, 2017 · 9 comments

Comments

@jon-turney
Copy link
Member

jon-turney commented Sep 15, 2017

An example extracted from xserver meson.build, where we had [] assigned to variable used for a dependency, to represent a null value

gbm_dep = []
if build_glamor
    gbm_dep = dependency('gbm', version: '>= 10.2', required: false)
endif
[hundreds of lines later...]
if gbm_dep.found()
   subdir('glamor_egl')
endif
[more tests of gbm_dep.found() later...]

This fails (in some configurations) withArrays do not have a method called 'found'.

One can write:

gbm_dep = dependency('', required: false)
if build_glamor
    gbm_dep = dependency('gbm', version: '>= 10.2', required: false)
endif

to get a dependency object for which found() is always false if the option is disabled, but this is undocumented, and has the side-effect of

Dependency  found: NO

during configuration

@nirbheek
Copy link
Member

See: #1753

@jon-turney
Copy link
Member Author

jon-turney commented Sep 28, 2017

Yuck.

Also, this isn't a general solution. Dependency objects support other methods (get_pkgconfig_variable(), type_name(), version()) and attempts to use those methods on an empty list will fail similarly. Dependency objects might also learn additional methods in the future.

From a consistency point of view, having this be a dependency object, rather than have lists sometimes behave a bit like a dependency object, seems preferable...

@jon-turney
Copy link
Member Author

jon-turney commented Sep 28, 2017

I notice that buried in the documentation for executable() is the sentence:

the best way to handle a 'disabled dependency' is by assigning an empty list [] to it and passing it like any other dependency to the dependencies: keyword argument.

I'd suggest removing this.

@nirbheek
Copy link
Member

Dependency objects support other methods (get_pkgconfig_variable(), type_name(), version()) and attempts to use those methods on an empty list will fail similarly

Those won't work on not-found dependencies, so you always have to wrap them inside a .found() call anyway.

From a consistency point of view, having this be a dependency object, rather than have lists sometimes behave a bit like a dependency object, seems preferable...

Later in the PR, we talk about how having a dependencies() object that is a collection of dependencies is probably a better approach. However, we can also allow never_dep = dependency() as a way of having a 'not-found' 'not-required' dep.

I'd suggest removing this.

Why? This works just fine for lists of dependencies.

jon-turney added a commit to jon-turney/meson that referenced this issue Oct 20, 2017
Document the dependency('', required:false) usage.  Add dependency() as a
shorthand for that.

Fixes mesonbuild#2324
jon-turney added a commit to jon-turney/meson that referenced this issue Oct 20, 2017
Document the dependency('', required:false) usage.  Add dependency() as a
shorthand for that.

Avoid emitting 'Dependency  found: NO' in either case.

Fixes mesonbuild#2324
@jon-turney
Copy link
Member Author

Dependency objects support other methods (get_pkgconfig_variable(), type_name(), version()) and attempts to use those methods on an empty list will fail similarly

Those won't work on not-found dependencies, so you always have to wrap them inside a .found() call anyway.

Are you sure?

$ cat meson.build
project('test')
dep = dependency('', required:false)
message(dep.type_name())
message(dep.get_pkgconfig_variable('libs'))
message(dep.version())

$ meson build
The Meson build system
Version: 0.42.1
Source dir: /test
Build dir: /test/build
Build type: native build
Project name: test
Build machine cpu family: x86_64
Build machine cpu: x86_64
Found pkg-config: /usr/bin/pkg-config (0.29.1)
Dependency  found: NO
Message: pkgconfig
Message:
Message:
Build targets in project: 0

The documentation explicitly mentions that calling get_pkgconfig_variable() on a non-pkgconfig dependency is an error (and indeed it does cause an NotImplementedError exception), but doesn't exclude this usage .

@jon-turney
Copy link
Member Author

I'd suggest removing this.

Why? This works just fine for lists of dependencies.

This tells people to write:

if some_condition
  dep = []
else
  dep = dependency('some_dependency')
endif
executable(..., dependencies: dep, ...)

This is not the best way to handle this, because this is just a dep.found() away from this issue.

jon-turney added a commit to jon-turney/meson that referenced this issue Oct 23, 2017
Document the dependency('', required:false) usage.  Add dependency() as a
shorthand for that.

Avoid emitting 'Dependency  found: NO' in either case.

Fixes mesonbuild#2324

v2:
Don't accidentally permit dependency('', 'foo', 'bar', 'baz',
required:false)
Set type_name() to 'not-found'
jon-turney added a commit to jon-turney/meson that referenced this issue Oct 23, 2017
Document the dependency('', required:false) usage.  Add dependency() as a
shorthand for that.

Avoid emitting 'Dependency  found: NO' in either case.

Fixes mesonbuild#2324

v2:
Don't accidentally permit dependency('', 'foo', 'bar', 'baz',
required:false)
Set type_name() to 'not-found'
jon-turney added a commit to jon-turney/mesa that referenced this issue Nov 13, 2017
It's ok to use an empty list for dependencies:, but it's not ok to try to
use the found() method of it.

See also mesonbuild/meson#2324
jon-turney added a commit to jon-turney/mesa that referenced this issue Nov 13, 2017
It's ok to use an empty list for dependencies:, but it's not ok to try to
use the found() method of it.

See also mesonbuild/meson#2324
@nirbheek
Copy link
Member

The documentation explicitly mentions that calling get_pkgconfig_variable() on a non-pkgconfig dependency is an error (and indeed it does cause an NotImplementedError exception), but doesn't exclude this usage .

That works right now because the underlying implementation is a PkgConfigDependency. When #2512 lands, get_configtool_variable() will fail on this object.

Relatedly, another use-case for a 'disabled dependency' is platform-specific internal dependencies such as gio-windows and gio-unix in glib. Only one of those two are available depending on whether you're on Windows or not.

@dcbaker
Copy link
Member

dcbaker commented Nov 28, 2017

@nirbheek, I've had this thought that really the get_configtool_variable/get_pkgconfig_variable situation could be improved via a generic mechanism, something like:
get_variable(pkgconfig : 'foo', configtool : 'foobar', fallback : 'something'), where each mechanism to attempt would be a keyeword argument, with the option of a fallback if they all fail (or without fallback the build would error).

I really like the idea of the empty dependency, it would allow the mesa build to be simplified as we have several optional dependencies and a lot of platform specific dependencies, and replacing checks like if dep_foo != [] and dep_foo.found() with if dep_foo.found() would be awesome.

@jon-turney
Copy link
Member Author

Closed by #2615

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants