containers / bubblewrap

Low-level unprivileged sandboxing tool used by Flatpak and similar projects
Other
3.75k stars 230 forks source link

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

Closed eli-schwartz closed 7 months ago

eli-schwartz commented 8 months ago

https://github.com/containers/bubblewrap/blob/7816e0129887c0da3f442244db24edc6e8a029a4/completions/bash/meson.build#L12-L18

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 commented 8 months ago

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 commented 8 months ago

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

smcv commented 8 months ago

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 commented 8 months ago

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:

eli-schwartz commented 7 months ago

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:

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 commented 7 months ago

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 https://github.com/scop/bash-completion/commit/8c5302581ad20954cfa164dcae26f1ed6277dd90, I think).

eli-schwartz commented 7 months ago

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