meson icon indicating copy to clipboard operation
meson copied to clipboard

`dep.get_variable()` for qmake

Open bb010g opened this issue 4 years ago • 6 comments

dependencies/configtool: customizable tool args

factor out ConfigToolDependency.get_configtool_args_for_variable() from ConfigToolDependency.get_configtool_variable()

This facilitates getting variables from config tools with non-standard argument syntax, both in Python and in Meson's specification language (via dep.get_variable() or dep.get_configtool_variable()), avoiding duplication of error handling & further parsing of tool results that would come from overriding the whole ConfigToolDependency.get_configtool_variable() function.

dependencies/qt: dep.get_variable() for qmake

To get config tool variables with qmake, qmake --{variable_name} cannot be used; instead, qmake -query {variable_name} must be used.

This customizes ConfigToolDependency.get_configtool_args_for_variable() via QmakeQtDependency.get_configtool_args_for_variable() to support dep.get_variable() and dep.get_configtool_variable() for qmake dependencies.

Addresses https://github.com/mesonbuild/meson/issues/8490. For example, qt5_dep.get_variable(configtool : 'QT_INSTALL_PLUGINS) as suggested by @goulf-3m actually works now.

Follow up to https://github.com/mesonbuild/meson/pull/8568.

Refactoring QmakeQtDependency.__init__() to use self.get_configtool_variable() or self.get_configtool_args_for_variable() was considered, but given that it's a special use of self.get_config_value() that eagerly queries all variables at once, which self.get_configtool_variable() doesn't currently support, the function was left alone.

bb010g avatar Dec 14 '21 22:12 bb010g

The changes themselves look reasonable enough, but the commit messages are... very information-dense.

Describing the exact implementation classes being modified is generally not necessary, and may detract from readability in git log. This information is already available in the change diff, and doesn't aid the reader in the meta-knowledge of what effective difference the change makes, and why, nor does it add contextual information lacking in the diff itself.

Addresses <https://github.com/mesonbuild/meson/issues/8490> in the middle of a paragraph should be removed, and replaced by Fixes #8490 in its own paragraph at the end of the commit message. This avoids stray <> in the github UI, because raw links are expected by the renderer, and matches the expected pattern for github's auto-linkification keywords (this ensures that merging the PR marks the issue as closed).

eli-schwartz avatar Dec 14 '21 23:12 eli-schwartz

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.08%. Comparing base (a2934ca) to head (0bf9ac4). Report is 2897 commits behind head on master.

Files Patch % Lines
mesonbuild/dependencies/qt.py 66.66% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9728      +/-   ##
==========================================
- Coverage   67.08%   67.08%   -0.01%     
==========================================
  Files         404      404              
  Lines       85711    85719       +8     
  Branches    18907    18907              
==========================================
+ Hits        57501    57506       +5     
- Misses      23676    23679       +3     
  Partials     4534     4534              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov[bot] avatar Dec 14 '21 23:12 codecov[bot]

does this correctly support qmake --qtconf '<somefullpath' --query <variablename> ?

Neumann-A avatar Dec 15 '21 11:12 Neumann-A

The changes themselves look reasonable enough, but the commit messages are... very information-dense.

Describing the exact implementation classes being modified is generally not necessary, and may detract from readability in git log. This information is already available in the change diff, and doesn't aid the reader in the meta-knowledge of what effective difference the change makes, and why, nor does it add contextual information lacking in the diff itself.

Addresses <https://github.com/mesonbuild/meson/issues/8490> in the middle of a paragraph should be removed, and replaced by Fixes #8490 in its own paragraph at the end of the commit message. This avoids stray <> in the github UI, because raw links are expected by the renderer, and matches the expected pattern for github's auto-linkification keywords (this ensures that merging the PR marks the issue as closed).

@eli-schwartz I'll shorten those up; figured I'd lean generous on commit messages rather than possibly have to write up comprehensive ones later. :)

does this correctly support qmake --qtconf '<somefullpath' --query <variablename> ?

@Neumann-A I've tested only on Qt 5.15.7; --query is an invalid option for qmake. ~/Qt/5.15.7/clang_64/bin/qmake -query QT_INSTALL_LIBS -qtconf ~/Qt/5.15.7/clang_64/bin/qt.conf works as expected; ~/Qt/5.15.7/clang_64/bin/qmake -qtconf ~/Qt/5.15.7/clang_64/bin/qt.conf -query QT_INSTALL_LIBS exits with code 1, prints ***Unknown option -query to stdout, and prints help to stderr; ~/Qt/5.15.7/clang_64/bin/qmake --query QT_INSTALL_LIBS --qtconf ~/Qt/5.15.7/clang_64/bin/qt.conf exits with code 1, prints ***Unknown option --query to stdout, and prints help to stderr.

-qtconf <file> is currently not used by QmakeQtDependency; support for this option can be implemented in a separate patch set.

If any of this is preferable in Qt 6 or required in Qt 6, patches are welcome here.

bb010g avatar Dec 15 '21 21:12 bb010g

I think we're still just waiting on a resolution to @dcbaker's review here (and ideally a tweak to the commit message wording). Any news on an update?

eli-schwartz avatar Mar 22 '22 16:03 eli-schwartz

@bb010g still interested in finishing this PR? Seems rather close to being done.

tristan957 avatar Aug 07 '23 17:08 tristan957

Codecov Report

Attention: Patch coverage is 83.33333% with 1 line in your changes missing coverage. Please review.

Project coverage is 67.08%. Comparing base (a2934ca) to head (0bf9ac4). Report is 3038 commits behind head on master.

Files with missing lines Patch % Lines
mesonbuild/dependencies/qt.py 66.66% 1 Missing :warning:
Additional details and impacted files
@@            Coverage Diff             @@
##           master    #9728      +/-   ##
==========================================
- Coverage   67.08%   67.08%   -0.01%     
==========================================
  Files         404      404              
  Lines       85711    85719       +8     
  Branches    18907    18907              
==========================================
+ Hits        57501    57506       +5     
- Misses      23676    23679       +3     
  Partials     4534     4534              

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

codecov-commenter avatar Sep 26 '24 15:09 codecov-commenter