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

Fails to build with meson 1.3.0 rc1 due to broken bash-completion handling #609

Closed
eli-schwartz opened this issue Nov 1, 2023 · 7 comments · Fixed by mesonbuild/meson#12509

Comments

@eli-schwartz
Copy link

bash_completion_dir = bash_completion.get_variable(
default_value: '',
pkgconfig: 'completionsdir',
pkgconfig_define: [
'prefix', get_option('prefix'),
'datadir', get_option('prefix') / get_option('datadir'),
],

In meson 1.2.x, this executes:

pkg-config bash-completion --variable completionsdir --define-variable=prefix=/usr=datadir=/usr/share

Which achieves nothing in the way of passing pkgconfig defines. completionsdir doesn't get datadir redefined, so it prints completionsdir unmodified.

In meson 1.3.0 due to a refactor this actually simply errors, based on the assumption that "users weren't supposed to do it". There may be some truth to that. If you'd flipped the order around, then this would instead be:

pkg-config bash-completion --variable completionsdir --define-variable=datadir=/usr/share=prefix=/usr

On my system, the resulting value of bash_completion_dir would then be:

/usr/share=prefix=/usr/bash-completion/completions

which is pretty badly broken, I daresay

Discovered in https://bugs.gentoo.org/916576
/cc @thesamesam

@eli-schwartz
Copy link
Author

I'm not completely positive what to do here. Obviously, meson 1.3.0 fails to successfully run meson install when 1.2.0 did, but on the other hand, this effectively totally ignored the value of pkgconfig_define and produced results you probably did not. Should we restore the old behavior? Or....

@smcv
Copy link
Collaborator

smcv commented Nov 2, 2023

Does Meson provide a way to do what the author of this code was clearly trying to do, namely define two variables?

@smcv
Copy link
Collaborator

smcv commented Nov 2, 2023

I'm very tempted to resolve this by not using pkgconfig_define, and instead assuming that the correct directory for bash completions within our ${prefix} is always going to be ${datadir}/bash-completion.

@eli-schwartz
Copy link
Author

Does Meson provide a way to do what the author of this code was clearly trying to do, namely define two variables?

I am not too sure that defining two variables was a useful thing for the author of this code to do, honestly. The definition that this code was trying to make for datadir (which was get_option('prefix') / get,_option('datadir')) would:

  • always resolve to an absolute path
  • always totally override any internal usage of ${prefix} by the pkg-config file's own variable definition of datadir=.

@eli-schwartz
Copy link
Author

ping. I want to resolve this one way or another in time for the final meson release, but I'm not sure what the correct solution is. I'm trying to decide whether:

  • we need to catch this case, warn that it effectively sets nothing, and carry on
  • to do something constructive with multiple defines, and warn that your meson_version is way too old to use the functionality you tried using

If there is something I'm missing about defining two defines, I'd be happy to hear it -- at the moment I'm inclined to choose option 1.

@smcv
Copy link
Collaborator

smcv commented Nov 13, 2023

I think the reasoning for this may have been that current versions of bash-completion have completionsdir=${datadir}/bash-completion/completions, but old versions had something more like ${prefix}/share/bash-completion/completions, and either way, if bubblewrap's prefix is (let's say) /usr/local then we want to install in /usr/local/share/bash-completion/completions?

The definition that this code was trying to make for datadir (which was get_option('prefix') / get_option('datadir')) would always resolve to an absolute path

That's fine, the intention is to get everything installed by bubblewrap to be below bubblewrap's prefix, even if that differs from bash-completion's prefix (for example /usr/local and /usr respectively).

[and] always totally override any internal usage of ${prefix} by the pkg-config file's own variable definition of datadir=

That's fine, the definition used for prefix is consistent with the one used in datadir anyway. The only time this would be harmful is if it was contradictory, which it isn't.

The only time our redefinition of prefix would be used (if this worked as intended) is if bash-completion's pkg-config didn't respect ${datadir} and included ${prefix} directly, as seen in older versions of bash-completion (those being versions before scop/bash-completion@8c53025, I think).

@eli-schwartz
Copy link
Author

Gotcha, thanks for clarifying. I agree this makes sense and should be possible, so that's what I'll do.

eli-schwartz added a commit to eli-schwartz/meson that referenced this issue Nov 13, 2023
It was previously impossible to do this:

```
dep.get_pkgconfig_variable(
    'foo',
    define_variable: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

since get_pkgconfig_variable mandated exactly two (if any) arguments.

However, you could do this:
```
dep.get_variable(
    'foo',
    pkgconfig_define: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

It would silently do the wrong thing, by defining "prefix" as
`/usr=datadir=/usr/share`, which might not "matter" if only datadir was
used in the "foo" variable as the unmodified value might be adequate.

The actual intention of anyone writing such a meson.build is that they
aren't sure whether the .pc file uses ${prefix} or ${datadir} (or which
one gets used, might have changed between versions of that .pc file,
even).

A recent refactor made this into a hard error, which broke some projects
that were doing this and inadvertently depending on some .pc file that
only used the second variable. (This was "fine" since the result was
essentially meaningful, and even resulted in behavior identical to the
intended behavior if both projects were installed into the same prefix
-- in which case there's nothing to remap.)

Re-allow this. There are two ways we could re-allow this:
- ignore it with a warning
- add a new feature to allow actually doing this

Since the use case which triggered this bug actually has a pretty good
reason to want to do this, it makes sense to add the new feature.

Fixes https://bugs.gentoo.org/916576
Fixes containers/bubblewrap#609
eli-schwartz added a commit to eli-schwartz/meson that referenced this issue Nov 14, 2023
It was previously impossible to do this:

```
dep.get_pkgconfig_variable(
    'foo',
    define_variable: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

since get_pkgconfig_variable mandated exactly two (if any) arguments.

However, you could do this:
```
dep.get_variable(
    'foo',
    pkgconfig_define: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

It would silently do the wrong thing, by defining "prefix" as
`/usr=datadir=/usr/share`, which might not "matter" if only datadir was
used in the "foo" variable as the unmodified value might be adequate.

The actual intention of anyone writing such a meson.build is that they
aren't sure whether the .pc file uses ${prefix} or ${datadir} (or which
one gets used, might have changed between versions of that .pc file,
even).

A recent refactor made this into a hard error, which broke some projects
that were doing this and inadvertently depending on some .pc file that
only used the second variable. (This was "fine" since the result was
essentially meaningful, and even resulted in behavior identical to the
intended behavior if both projects were installed into the same prefix
-- in which case there's nothing to remap.)

Re-allow this. There are two ways we could re-allow this:
- ignore it with a warning
- add a new feature to allow actually doing this

Since the use case which triggered this bug actually has a pretty good
reason to want to do this, it makes sense to add the new feature.

Fixes https://bugs.gentoo.org/916576
Fixes containers/bubblewrap#609
eli-schwartz added a commit to eli-schwartz/meson that referenced this issue Nov 14, 2023
It was previously impossible to do this:

```
dep.get_pkgconfig_variable(
    'foo',
    define_variable: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

since get_pkgconfig_variable mandated exactly two (if any) arguments.

However, you could do this:
```
dep.get_variable(
    'foo',
    pkgconfig_define: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

It would silently do the wrong thing, by defining "prefix" as
`/usr=datadir=/usr/share`, which might not "matter" if only datadir was
used in the "foo" variable as the unmodified value might be adequate.

The actual intention of anyone writing such a meson.build is that they
aren't sure whether the .pc file uses ${prefix} or ${datadir} (or which
one gets used, might have changed between versions of that .pc file,
even).

A recent refactor made this into a hard error, which broke some projects
that were doing this and inadvertently depending on some .pc file that
only used the second variable. (This was "fine" since the result was
essentially meaningful, and even resulted in behavior identical to the
intended behavior if both projects were installed into the same prefix
-- in which case there's nothing to remap.)

Re-allow this. There are two ways we could re-allow this:
- ignore it with a warning
- add a new feature to allow actually doing this

Since the use case which triggered this bug actually has a pretty good
reason to want to do this, it makes sense to add the new feature.

Fixes https://bugs.gentoo.org/916576
Fixes containers/bubblewrap#609
gerioldman pushed a commit to gerioldman/meson that referenced this issue Jul 2, 2024
It was previously impossible to do this:

```
dep.get_pkgconfig_variable(
    'foo',
    define_variable: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

since get_pkgconfig_variable mandated exactly two (if any) arguments.

However, you could do this:
```
dep.get_variable(
    'foo',
    pkgconfig_define: ['prefix', '/usr', 'datadir', '/usr/share'],
)
```

It would silently do the wrong thing, by defining "prefix" as
`/usr=datadir=/usr/share`, which might not "matter" if only datadir was
used in the "foo" variable as the unmodified value might be adequate.

The actual intention of anyone writing such a meson.build is that they
aren't sure whether the .pc file uses ${prefix} or ${datadir} (or which
one gets used, might have changed between versions of that .pc file,
even).

A recent refactor made this into a hard error, which broke some projects
that were doing this and inadvertently depending on some .pc file that
only used the second variable. (This was "fine" since the result was
essentially meaningful, and even resulted in behavior identical to the
intended behavior if both projects were installed into the same prefix
-- in which case there's nothing to remap.)

Re-allow this. There are two ways we could re-allow this:
- ignore it with a warning
- add a new feature to allow actually doing this

Since the use case which triggered this bug actually has a pretty good
reason to want to do this, it makes sense to add the new feature.

Fixes https://bugs.gentoo.org/916576
Fixes containers/bubblewrap#609
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 a pull request may close this issue.

2 participants