Consolidate reporting the result of a dependency check #3657
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Rather than spamming issues, some other things noted in passing:
|
HAHA yes. We were supposed to consolidate the code for tool searching and version checking across all dependencies instead of duplicating it in each, but we never got around to doing it. |
jon-turney
commented
Jun 1, 2018
jon-turney
force-pushed
the
consolidate-dependency-check-report
branch
2 times, most recently
from
0d32b53 to
d31c44c
Compare
jon-turney
force-pushed
the
consolidate-dependency-check-report
branch
from
d31c44c to
e31609f
Compare
|
Rebased to resolve conflict. |
|
Is this ready for review? |
Sure. I'm not sure it's important for 0.47.0, though, since it's just clean up. |
jon-turney
force-pushed
the
consolidate-dependency-check-report
branch
from
e31609f to
bf58852
Compare
|
⚔️ conflicts. |
jon-turney
force-pushed
the
consolidate-dependency-check-report
branch
from
bf58852 to
39fa94c
Compare
|
Rebased to resolve conflict. |
libwmf and pcap allegedly support method:extraframework, but this does nothing but return not-found. Possibly cut-and-pasted from elsewhere?
dub dependencies allegedly support method:pkgconfig, but this is meaningless as it's only ever used when the dub method is explicitly requested. Possibly cut-and-pasted from elsewhere?
…_dependency() find_external_dependency() now makes and iterates over a list of callables which are constructors with bound arguments for the dependency objects we are going to attempt to make, so we can consolidate reporting on these attempts and handling failures in that function.
Give ConfigToolDependency() a finish_init callback, so that tool-specific initialization can be called from the constructor, rather than after construction in the factory class. v2: finalize -> finish_init for clarity
This is now done by find_external_dependency() in all cases I can't help but think this perhaps should be in a few more places...
If successful, we should identify the method which was successful If successful, we should report the version found (if known) If failing, we should identify the methods we tried Some dependency detectors which had no reporting now gain it There's all kinds of complexities, inconsistencies and special cases hidden in the existing behaviour, e.g.: - boost reports modules requested, and BOOST_ROOT (if set) - gtest/gmock report if they are a prebuilt library or header only - mpi reports the language - qt reports modules requested, and the config tool used or tried - configtool reports the config tool used - llvm reports if missing modules are optional (one per line) We add some simple hooks to allow the dependency object to expose the currently reported information into the consolidated reporting Note that PkgConfigDependency() takes a silent: keyword which is used internallly to suppress reporting. This behaviour isn't needed in find_external_dependency().
If we aren't cross-building, this distinction isn't very interesting (Most dependencies didn't bother reporting this. pkgconfig did it unconditionally, and Qt reported as 'native' or 'cross' depending on is_cross_build(), i.e. ignoring the native: keyword)
Display dependency name with the correct casing, for the (extended) list of dependencies we have a preferred casing for.
jon-turney
force-pushed
the
consolidate-dependency-check-report
branch
from
39fa94c to
c333561
Compare
|
This PR is quite difficult to review, could you paste some examples of what the output looks like? I think that should be enough to do a review. |
The idea behind this change wasn't really to change the output, I've tried to keep it generally the same, but that it's very hard to make any change to the output, as each The output discussed in #3095 changes like this: before: after: diff: --- before 2018-08-08 21:24:13.567000000 +0100
+++ after 2018-08-08 21:14:14.003000000 +0100
@@ -10,10 +10,11 @@
Build machine cpu family: x86_64
Build machine cpu: x86_64
Found pkg-config: /usr/bin/pkg-config (0.29.1)
-Native dependency glib-2.0 found: YES 2.56.1
-Native dependency gobject-2.0 found: YES 2.56.1
-Native dependency gio-2.0 found: YES 2.56.1
-Native dependency libsoup-2.4 found: YES 2.62.1
+Dependency glib-2.0 found: YES 2.56.1
+Dependency gobject-2.0 found: YES 2.56.1
+Dependency gio-2.0 found: YES 2.56.1
+Dependency libsoup-2.4 found: YES 2.62.1
+Dependency vsgi found: NO (tried pkgconfig) <--- added
Looking for a fallback subproject for the dependency vsgi
Cloning into 'vsgi'...
remote: Counting objects: 5754, done.
@@ -33,14 +34,15 @@
|Dependency glib-2.0 found: YES (cached)
|Dependency gobject-2.0 found: YES (cached)
|Dependency gio-2.0 found: YES (cached)
-|Native dependency gio-unix-2.0 found: YES 2.56.1
-|Native dependency gmodule-2.0 found: YES 2.56.1
+|Dependency gio-unix-2.0 found: YES 2.56.1
+|Dependency gmodule-2.0 found: YES 2.56.1
|Dependency libsoup-2.4 found: YES (cached)
-|Dependency libsystemd found: NO
+|Dependency libsystemd found: NO (tried pkgconfig)
+|Dependency openssl found: NO (tried pkgconfig)
Couldn't use fallback subproject in subprojects/vsgi for the dependency vsgi
-Reason: subprojects/vsgi/meson.build:20: Native dependency 'openssl' not found
+Reason: subprojects/vsgi/meson.build:20: Dependency "openssl" not found, tried pkgconfig
-meson.build:17:0: ERROR: Native dependency 'vsgi' not found
+meson.build:17:0: ERROR: Dependency "vsgi" not found, tried pkgconfig
A full log can be found at /home/jon/src/valum/_build/meson-logs/meson-log.txtIt also suppresses cross/native reporting as uninteresting when we are not cross-building, corrects it for Qt, and extends it to all dependency classes. It also should make it much more straightforward to extend the implementation of the |
nirbheek
approved these changes
Aug 11, 2018
|
Merged with a merge commit; should have no functionality changes or new features. |
|
Thanks. I'll probably take a look at seeing if I can factor out |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivated by the observation in #3095 that currently, we don't report if a dependency was not found before trying a fallback subproject.
(at the moment, if a dependency isn't found,
find_external_dependency()either (i) logs that it wasn't found, if it is not required, or (ii) raises an exception, if it is required (that exception being held byfunc_dependency()while it checks if a workable fallback exists))This is hard to fix straightforwardly because, at the moment, the
ExternalDependency-derived constructor called byfind_external_dependency()is responsible for reporting success or raising an exception if not found, but required.This patch series consolidates that reporting and checking in
find_external_dependency().This enables always logging the result of the dependency check, as well as removing some inconsistencies in how those checks are reported.
This might also help #3376 to simply report that "checking for dependency x was skipped, because the requirement for it was disabled"