modules/gnome: fix missing install_dir, again, harder #9484

dcbaker · 2021-10-28T17:55:23Z

It turns out this could be missing in GResource*Target as well, due
mostly to the same problem, side effects of mutating a shared
dictionary; though it could also happen with a specific set of keywords
given and others omitted.

Fixes #9350, again

mesonbuild/modules/gnome.py

xclaesse · 2021-10-28T17:58:42Z

Other than that, LGTM.

It turns out this could be missing in GResource*Target as well, due mostly to the same problem, side effects of mutating a shared dictionary; though it could also happen with a specific set of keywords given and other omitted. Fixes mesonbuild#9350

codecov · 2021-10-28T18:19:49Z

Codecov Report

Merging #9484 (bba5600) into master (ae35b1f) will increase coverage by 0.00%.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##           master    #9484   +/-   ##
=======================================
  Coverage   67.32%   67.32%           
=======================================
  Files         396      396           
  Lines       85476    85480    +4     
  Branches    17661    17661           
=======================================
+ Hits        57546    57550    +4     
  Misses      23245    23245           
  Partials     4685     4685

Impacted Files	Coverage Δ
mesonbuild/modules/gnome.py	`71.31% <100.00%> (+0.04%)`	⬆️
mesonbuild/scripts/vcstagger.py	`87.50% <0.00%> (-4.17%)`	⬇️
modules/gnome.py	`70.56% <0.00%> (+0.04%)`	⬆️
mesonbuild/mesonlib/universal.py	`81.65% <0.00%> (+0.08%)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ae35b1f...bba5600. Read the comment docs.

xclaesse · 2021-10-28T19:03:07Z

Looks good, LGTM.

justinkb · 2021-11-26T16:49:29Z

with these changes (and the backports to 0.60.x), gjs 1.70.0 build no longer works properly

it complains about install_dir needing to be specified here

meson.build:563:0: ERROR: "install_dir" must be specified when installing a target

gjs_private_gir = gnome.generate_gir(libgjs,
    includes: ['GObject-2.0', 'Gio-2.0'], sources: libgjs_private_sources,
    namespace: 'GjsPrivate', nsversion: '1.0', identifier_prefix: 'Gjs',
    symbol_prefix: 'gjs_', extra_args: '--warn-error', install: true,
    install_dir_gir: false, install_dir_typelib: pkglibdir / 'girepository-1.0')

eli-schwartz · 2021-11-26T17:31:15Z

Since #9520,

meson.build:580:0: ERROR: gnome.generate_gir keyword argument 'install_dir_gir' was of type 'bool' but should have been one of: str, NoneType

This was never legal, I think. What was the point of passing "false" there?

justinkb · 2021-11-26T17:34:19Z

I have just finished doing some digging, it is an issue with gjs meson.build. (Present since the first version where the switch to meson was made). the .gir file GjsPrivate-1.0.gir is missing on every package on every distribution that's using meson to build gjs). I have made a patch, and will submit it to gjs gitlab

edit: https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/693 fwiw

ptomato · 2021-11-27T20:28:41Z

This was never legal, I think. What was the point of passing "false" there?

I was told that passing false here was the intended way of installing the typelib without installing the GIR: #1014 (comment)

Is there any other recommended way to install the typelib without installing the GIR? If that is no longer possible, then I think this is a regression in Meson's gnome module. GJS is not the only project that generates a typelib with private functions and therefore declines to install a GIR that other programs could consume.

ptomato · 2021-11-27T23:40:40Z

Someone figured out that install_dir_gir: [false] still works for this purpose. Could this (or any other way of achieving it that you prefer) be documented and a regression test added for it?

eli-schwartz · 2021-11-28T00:27:53Z

That won't work either, at least on meson-git master:

meson.build:580:0: ERROR: gnome.generate_gir keyword argument 'install_dir_gir' was of type 'list' but should have been one of: str, NoneType

Fiddling with the types in order to trick the parser isn't a robust solution. Anything that should work, should work without hiding it inside an array -- and anything that shouldn't work, can't be relied on to work in the future just because the internal implementation currently swallows an array of boolean better than a boolean.

However, I guess documented or not, it does seem that the code allowed it due to the use of kwargs.get() and actually it does seem like there is a good reason to allow this. Specifically, the rationale of #1469 implies that it should be explicitly possible, even though that came after install_dir_gir's implicit behavior.

The real problem I guess is that this wasn't documented (even though that PR documented it for vala), so the fact that it "should" work wasn't obvious... and then this got fundamentally broken by commit be1d013 which adds typechecking to this function but assumed it only accepted strings as implied by the docs.

@dcbaker sorry, but this is broken a third time...

The good news is that every function that gets properly typechecked then has a pure code description of what it should and does accept, so once we get this non-broken it should remain that way.

eli-schwartz · 2021-11-28T00:30:10Z

Also per the comment on the gjs MR:

given that it looks like that 4 year old proposal to split generate_gir into two never gained traction.

I think the main problem is no one ever submitted a PR for it. :( It seems like a basically good idea.

dcbaker · 2021-11-28T03:32:58Z

The fix should be trivial, I'll have a look at it tomorrow or Monday

…r and *_typelib generate_gir forces building both the typelib and gir, and some people only want one or the other (probably only the typelib?) which means flagging the other as install_dir: false in the same way custom_target supports. As this always worked, albeit undocumented, make sure it keeps working. It's pretty reasonable to allow, anyway. Fixes mesonbuild#9484 (comment)

generate_gir forces building both the typelib and gir, and some people only want one or the other (probably only the typelib?) which means flagging the other as install_dir: false in the same way custom_target supports. As this always worked, albeit undocumented, make sure it keeps working. It's pretty reasonable to allow, anyway. Fixes mesonbuild#9484 (comment)

…r and *_typelib generate_gir forces building both the typelib and gir, and some people only want one or the other (probably only the typelib?) which means flagging the other as install_dir: false in the same way custom_target supports. As this always worked, albeit undocumented, make sure it keeps working. It's pretty reasonable to allow, anyway. Fixes mesonbuild#9484 (comment)

eli-schwartz · 2022-01-11T23:55:23Z

That won't work either, at least on meson-git master:
meson.build:580:0: ERROR: gnome.generate_gir keyword argument 'install_dir_gir' was of type 'list' but should have been one of: str, NoneType
Fiddling with the types in order to trick the parser isn't a robust solution. Anything that should work, should work without hiding it inside an array -- and anything that shouldn't work, can't be relied on to work in the future just because the internal implementation currently swallows an array of boolean better than a boolean.

Apparently several projects did in fact use that broken hack anyway. @jbeich

linuxmint/cinnamon#10596
linuxmint/cjs#100
https://gitlab.gnome.org/GNOME/gjs/-/merge_requests/705

No idea if there are more.

ptomato · 2022-01-12T00:09:48Z

Yep, I was aware that we would have to revert that. I was just waiting to see what the resolution was going to be from this issue.

eli-schwartz · 2022-01-12T00:23:11Z

The resolution is, fixed in 0.60.3 (released December 22) and deprecated in 0.61.

In 0.61.0, gjs will print the following informational message at the end:

NOTICE: Future-deprecated features used:
 * 0.55.0: {'gnome.generate_gir argument --warn-error'}
 * 0.56.0: {'meson.build_root', 'Dependency.get_pkgconfig_variable'}
 * 0.61.0: {'"gnome.generate_gir" keyword argument "install_dir_gir" value "False"'}

That last one wants you to switch:

-install_dir_gir: false
+install_gir: false

As soon as you bump the minimum version of meson to 0.61

…r and *_typelib generate_gir forces building both the typelib and gir, and some people only want one or the other (probably only the typelib?) which means flagging the other as install_dir: false in the same way custom_target supports. As this always worked, albeit undocumented, make sure it keeps working. It's pretty reasonable to allow, anyway. Fixes mesonbuild#9484 (comment)

dcbaker added the module:gnome label Oct 28, 2021

dcbaker added this to the 0.60.1 milestone Oct 28, 2021

dcbaker requested a review from jpakkane as a code owner October 28, 2021 17:55

xclaesse reviewed Oct 28, 2021

View reviewed changes

mesonbuild/modules/gnome.py Outdated Show resolved Hide resolved

dcbaker force-pushed the submit/gnome-fix-install-dir-harder branch from 861dd42 to acd8aed Compare October 28, 2021 18:10

modules/gnome: use install_dir = [] instead of false

bba5600

xclaesse merged commit c8ea5df into mesonbuild:master Oct 28, 2021

dcbaker deleted the submit/gnome-fix-install-dir-harder branch October 28, 2021 22:23

xclaesse mentioned this pull request Oct 30, 2021

Fix KeyError: 'install_dir' in build.py #9496

Closed

eli-schwartz mentioned this pull request Nov 28, 2021

gnome module: document and fix install_dir x3, by allowing false *_gir and *_typelib #9666

Merged

eli-schwartz mentioned this pull request Nov 28, 2021

gnome module: fix install_dir x3, by allowing false *_gir and *_typelib #9667

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

modules/gnome: fix missing install_dir, again, harder #9484

modules/gnome: fix missing install_dir, again, harder #9484

dcbaker commented Oct 28, 2021

xclaesse commented Oct 28, 2021

codecov bot commented Oct 28, 2021 •

edited

xclaesse commented Oct 28, 2021

justinkb commented Nov 26, 2021

eli-schwartz commented Nov 26, 2021 •

edited

justinkb commented Nov 26, 2021 •

edited

ptomato commented Nov 27, 2021

ptomato commented Nov 27, 2021

eli-schwartz commented Nov 28, 2021

eli-schwartz commented Nov 28, 2021

dcbaker commented Nov 28, 2021

eli-schwartz commented Jan 11, 2022

ptomato commented Jan 12, 2022

eli-schwartz commented Jan 12, 2022

modules/gnome: fix missing install_dir, again, harder #9484

modules/gnome: fix missing install_dir, again, harder #9484

Conversation

dcbaker commented Oct 28, 2021

xclaesse commented Oct 28, 2021

codecov bot commented Oct 28, 2021 • edited

Codecov Report

xclaesse commented Oct 28, 2021

justinkb commented Nov 26, 2021

eli-schwartz commented Nov 26, 2021 • edited

justinkb commented Nov 26, 2021 • edited

ptomato commented Nov 27, 2021

ptomato commented Nov 27, 2021

eli-schwartz commented Nov 28, 2021

eli-schwartz commented Nov 28, 2021

dcbaker commented Nov 28, 2021

eli-schwartz commented Jan 11, 2022

ptomato commented Jan 12, 2022

eli-schwartz commented Jan 12, 2022

codecov bot commented Oct 28, 2021 •

edited

eli-schwartz commented Nov 26, 2021 •

edited

justinkb commented Nov 26, 2021 •

edited