From c7d71c79434d98c280121ac1a20faca8fd512e40 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Sun, 7 May 2017 02:01:41 +0530 Subject: dependencies: Fix caching of native/cross dependencies All our cached_dep magic was totally useless since we ended up using the same identifier for native and cross deps. Just nuke all this cached_dep code since it is very error-prone and improve the identifier generation instead. For instance, this is broken *right now* with the `type_name` kwarg. Add a bunch of tests to ensure that all this actually works... Closes https://github.com/mesonbuild/meson/issues/1736 --- mesonbuild/interpreter.py | 12 ++++-------- 1 file changed, 4 insertions(+), 8 deletions(-) (limited to 'mesonbuild/interpreter.py') diff --git a/mesonbuild/interpreter.py b/mesonbuild/interpreter.py index 80d482e..886f200 100644 --- a/mesonbuild/interpreter.py +++ b/mesonbuild/interpreter.py @@ -1858,7 +1858,6 @@ class Interpreter(InterpreterBase): if '<' in name or '>' in name or '=' in name: raise InvalidArguments('Characters <, > and = are forbidden in dependency names. To specify' 'version\n requirements use the \'version\' keyword argument instead.') - identifier = dependencies.get_dep_identifier(name, kwargs) # Check if we want this as a cross-dep or a native-dep # FIXME: Not all dependencies support such a distinction right now, # and we repeat this check inside dependencies that do. We need to @@ -1868,10 +1867,14 @@ class Interpreter(InterpreterBase): want_cross = not kwargs['native'] else: want_cross = is_cross + identifier = dependencies.get_dep_identifier(name, kwargs, want_cross) # Check if we've already searched for and found this dep cached_dep = None if identifier in self.coredata.deps: cached_dep = self.coredata.deps[identifier] + # All other kwargs are handled in get_dep_identifier(). We have + # this here as a tiny optimization to avoid searching for + # dependencies that we already have. if 'version' in kwargs: wanted = kwargs['version'] found = cached_dep.get_version() @@ -1880,13 +1883,6 @@ class Interpreter(InterpreterBase): # Cached dep has the wrong version. Check if an external # dependency or a fallback dependency provides it. cached_dep = None - # Don't re-use cached dep if it wasn't required but this one is, - # so we properly go into fallback/error code paths - if kwargs.get('required', True) and not getattr(cached_dep, 'required', False): - cached_dep = None - # Don't reuse cached dep if one is a cross-dep and the other is a native dep - if not getattr(cached_dep, 'want_cross', is_cross) == want_cross: - cached_dep = None if cached_dep: dep = cached_dep -- cgit v1.1 From c650ba8928c6ded4adc7faf1bca3f28df8f47163 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Sun, 7 May 2017 03:53:00 +0530 Subject: interpreter: Typo in error message 'Non-existent' is the grammatically correct version. Also call it a 'build file' since that's what everyone calls it nowadays. --- mesonbuild/interpreter.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) (limited to 'mesonbuild/interpreter.py') diff --git a/mesonbuild/interpreter.py b/mesonbuild/interpreter.py index 886f200..46ec3f3 100644 --- a/mesonbuild/interpreter.py +++ b/mesonbuild/interpreter.py @@ -2226,7 +2226,7 @@ class Interpreter(InterpreterBase): absname = os.path.join(self.environment.get_source_dir(), buildfilename) if not os.path.isfile(absname): self.subdir = prev_subdir - raise InterpreterException('Nonexistent build def file %s.' % buildfilename) + raise InterpreterException('Non-existent build file {!r}'.format(buildfilename)) with open(absname, encoding='utf8') as f: code = f.read() assert(isinstance(code, str)) -- cgit v1.1 From 8cf29bd288cb67008a42a5c9503042f975c04a43 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Sun, 7 May 2017 11:15:47 +0530 Subject: Completely overhaul caching of external dependencies The old caching was a mess of spaghetti code layered over pasta code. The new code is well-commented, is clear about what it's trying to do, and uses a blacklist of keyword arguments instead of a whitelist while generating identifiers for dep caching which makes it much more robust for future changes. The only side-effect of forgetting about a new keyword argument would be that the dependency would not be cached unless the values of that keyword arguments were the same in the cached and new dependency. There are also more tests which identify scenarios that were broken earlier. --- mesonbuild/interpreter.py | 92 ++++++++++++++++++++++++++++++++++------------- 1 file changed, 67 insertions(+), 25 deletions(-) (limited to 'mesonbuild/interpreter.py') diff --git a/mesonbuild/interpreter.py b/mesonbuild/interpreter.py index 46ec3f3..c1926d0 100644 --- a/mesonbuild/interpreter.py +++ b/mesonbuild/interpreter.py @@ -23,7 +23,8 @@ from . import compilers from .wrap import wrap, WrapMode from . import mesonlib from .mesonlib import FileMode, Popen_safe, get_meson_script -from .dependencies import InternalDependency, Dependency, ExternalProgram +from .dependencies import ExternalProgram +from .dependencies import InternalDependency, Dependency, DependencyException from .interpreterbase import InterpreterBase from .interpreterbase import check_stringlist, noPosargs, noKwargs, stringArgs from .interpreterbase import InterpreterException, InvalidArguments, InvalidCode @@ -1852,12 +1853,16 @@ class Interpreter(InterpreterBase): def func_find_library(self, node, args, kwargs): mlog.log(mlog.red('DEPRECATION:'), 'find_library() is removed, use the corresponding method in compiler object instead.') - def func_dependency(self, node, args, kwargs): - self.validate_arguments(args, 1, [str]) - name = args[0] - if '<' in name or '>' in name or '=' in name: - raise InvalidArguments('Characters <, > and = are forbidden in dependency names. To specify' - 'version\n requirements use the \'version\' keyword argument instead.') + def _find_cached_dep(self, name, kwargs): + ''' + Check that there aren't any mismatches between the cached dep and the + wanted dep in terms of version and whether to use a fallback or not. + For instance, the cached dep and the wanted dep could have mismatching + version requirements. The cached dep did not search for a fallback, but + the wanted dep specifies a fallback. There are many more edge-cases. + Most cases are (or should be) documented in: + `test cases/linuxlike/5 dependency versions/meson.build` + ''' # Check if we want this as a cross-dep or a native-dep # FIXME: Not all dependencies support such a distinction right now, # and we repeat this check inside dependencies that do. We need to @@ -1868,52 +1873,89 @@ class Interpreter(InterpreterBase): else: want_cross = is_cross identifier = dependencies.get_dep_identifier(name, kwargs, want_cross) - # Check if we've already searched for and found this dep cached_dep = None + # Check if we've already searched for and found this dep if identifier in self.coredata.deps: cached_dep = self.coredata.deps[identifier] - # All other kwargs are handled in get_dep_identifier(). We have - # this here as a tiny optimization to avoid searching for - # dependencies that we already have. - if 'version' in kwargs: - wanted = kwargs['version'] - found = cached_dep.get_version() - if not cached_dep.found() or \ - not mesonlib.version_compare_many(found, wanted)[0]: - # Cached dep has the wrong version. Check if an external - # dependency or a fallback dependency provides it. - cached_dep = None + else: + # Check if exactly the same dep with different version requirements + # was found already. + # We only return early if we find a usable cached dependency since + # there might be multiple cached dependencies like this. + w = identifier[1] + for trial, trial_dep in self.coredata.deps.items(): + # trial[1], identifier[1] are the version requirements + if trial[0] != identifier[0] or trial[2:] != identifier[2:]: + continue + # The version requirements are the only thing that's different. + if trial_dep.found(): + # Cached dependency was found. We're close. + f = trial_dep.get_version() + if not w or mesonlib.version_compare_many(f, w)[0]: + # We either don't care about the version, or the + # cached dep version matches our requirements! Yay! + return identifier, trial_dep + elif 'fallback' not in kwargs: + # this cached dependency matched everything but was + # not-found. Tentatively set this as the dep to use. + # If no other cached dep matches, we will use this as the + # not-found dep. + cached_dep = trial_dep + # There's a subproject fallback specified for this not-found dependency + # which might provide it, so we must check it. + if cached_dep and not cached_dep.found() and 'fallback' in kwargs: + return identifier, None + # Either no cached deps matched the dep we're looking for, or some + # not-found cached dep matched and there is no fallback specified. + # Either way, we're done. + return identifier, cached_dep + + def func_dependency(self, node, args, kwargs): + self.validate_arguments(args, 1, [str]) + name = args[0] + if '<' in name or '>' in name or '=' in name: + raise InvalidArguments('Characters <, > and = are forbidden in dependency names. To specify' + 'version\n requirements use the \'version\' keyword argument instead.') + identifier, cached_dep = self._find_cached_dep(name, kwargs) if cached_dep: + if kwargs.get('required', True) and not cached_dep.found(): + m = 'Dependency {!r} was already checked and was not found' + raise DependencyException(m.format(name)) dep = cached_dep else: # We need to actually search for this dep exception = None dep = None - # If the fallback has already been configured (possibly by a higher level project) - # try to use it before using the native version + # If the dependency has already been configured, possibly by + # a higher level project, try to use it first. if 'fallback' in kwargs: dirname, varname = self.get_subproject_infos(kwargs) if dirname in self.subprojects: + subproject = self.subprojects[dirname] try: - dep = self.subprojects[dirname].get_variable_method([varname], {}) - dep = dep.held_object + # Never add fallback deps to self.coredata.deps + return subproject.get_variable_method([varname], {}) except KeyError: pass + # Search for it outside the project if not dep: try: dep = dependencies.find_external_dependency(name, self.environment, kwargs) - except dependencies.DependencyException as e: + except DependencyException as e: exception = e pass + # Search inside the projects list if not dep or not dep.found(): if 'fallback' in kwargs: fallback_dep = self.dependency_fallback(name, kwargs) if fallback_dep: + # Never add fallback deps to self.coredata.deps since we + # cannot cache them. They must always be evaluated else + # we won't actually read all the build files. return fallback_dep - if not dep: raise exception -- cgit v1.1 From 66f0ccb39b74f7d1022ec70fa9627bd6e4de8e29 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Sun, 7 May 2017 12:58:50 +0530 Subject: dependencies: Fix two more edge-cases in dependency searching Includes tests for both of them. --- mesonbuild/interpreter.py | 23 +++++++++++------------ 1 file changed, 11 insertions(+), 12 deletions(-) (limited to 'mesonbuild/interpreter.py') diff --git a/mesonbuild/interpreter.py b/mesonbuild/interpreter.py index c1926d0..0db097e 100644 --- a/mesonbuild/interpreter.py +++ b/mesonbuild/interpreter.py @@ -1882,25 +1882,24 @@ class Interpreter(InterpreterBase): # was found already. # We only return early if we find a usable cached dependency since # there might be multiple cached dependencies like this. - w = identifier[1] + wanted = identifier[1] for trial, trial_dep in self.coredata.deps.items(): # trial[1], identifier[1] are the version requirements if trial[0] != identifier[0] or trial[2:] != identifier[2:]: continue - # The version requirements are the only thing that's different. if trial_dep.found(): - # Cached dependency was found. We're close. - f = trial_dep.get_version() - if not w or mesonlib.version_compare_many(f, w)[0]: - # We either don't care about the version, or the - # cached dep version matches our requirements! Yay! + found = trial_dep.get_version() + if not wanted or mesonlib.version_compare_many(found, wanted)[0]: + # We either don't care about the version, or our + # version requirements matched the trial dep's version, + # and the trial dep was a found dep! return identifier, trial_dep - elif 'fallback' not in kwargs: - # this cached dependency matched everything but was - # not-found. Tentatively set this as the dep to use. - # If no other cached dep matches, we will use this as the - # not-found dep. + elif not trial[1]: + # If the not-found cached dep did not have any version + # requirements, this probably means the external dependency + # cannot be found. cached_dep = trial_dep + break # There's a subproject fallback specified for this not-found dependency # which might provide it, so we must check it. if cached_dep and not cached_dep.found() and 'fallback' in kwargs: -- cgit v1.1 From 830b44867cc3747f186c4a737675b067bb94fae2 Mon Sep 17 00:00:00 2001 From: Nirbheek Chauhan Date: Mon, 8 May 2017 02:00:36 +0530 Subject: dependencies: Only store found deps in the cache This simplifies everything since it means we will always search for the dependency again on the system if it wasn't found. This is particularly important when running `ninja reconfigure` with an edited PKG_CONFIG_PATH to point to a path that contains more pkg-config files. --- mesonbuild/interpreter.py | 37 +++++++------------------------------ 1 file changed, 7 insertions(+), 30 deletions(-) (limited to 'mesonbuild/interpreter.py') diff --git a/mesonbuild/interpreter.py b/mesonbuild/interpreter.py index 0db097e..5df26cc 100644 --- a/mesonbuild/interpreter.py +++ b/mesonbuild/interpreter.py @@ -1854,15 +1854,6 @@ class Interpreter(InterpreterBase): mlog.log(mlog.red('DEPRECATION:'), 'find_library() is removed, use the corresponding method in compiler object instead.') def _find_cached_dep(self, name, kwargs): - ''' - Check that there aren't any mismatches between the cached dep and the - wanted dep in terms of version and whether to use a fallback or not. - For instance, the cached dep and the wanted dep could have mismatching - version requirements. The cached dep did not search for a fallback, but - the wanted dep specifies a fallback. There are many more edge-cases. - Most cases are (or should be) documented in: - `test cases/linuxlike/5 dependency versions/meson.build` - ''' # Check if we want this as a cross-dep or a native-dep # FIXME: Not all dependencies support such a distinction right now, # and we repeat this check inside dependencies that do. We need to @@ -1880,33 +1871,17 @@ class Interpreter(InterpreterBase): else: # Check if exactly the same dep with different version requirements # was found already. - # We only return early if we find a usable cached dependency since - # there might be multiple cached dependencies like this. wanted = identifier[1] for trial, trial_dep in self.coredata.deps.items(): # trial[1], identifier[1] are the version requirements if trial[0] != identifier[0] or trial[2:] != identifier[2:]: continue - if trial_dep.found(): - found = trial_dep.get_version() - if not wanted or mesonlib.version_compare_many(found, wanted)[0]: - # We either don't care about the version, or our - # version requirements matched the trial dep's version, - # and the trial dep was a found dep! - return identifier, trial_dep - elif not trial[1]: - # If the not-found cached dep did not have any version - # requirements, this probably means the external dependency - # cannot be found. + found = trial_dep.get_version() + if not wanted or mesonlib.version_compare_many(found, wanted)[0]: + # We either don't care about the version, or our + # version requirements matched the trial dep's version. cached_dep = trial_dep break - # There's a subproject fallback specified for this not-found dependency - # which might provide it, so we must check it. - if cached_dep and not cached_dep.found() and 'fallback' in kwargs: - return identifier, None - # Either no cached deps matched the dep we're looking for, or some - # not-found cached dep matched and there is no fallback specified. - # Either way, we're done. return identifier, cached_dep def func_dependency(self, node, args, kwargs): @@ -1958,7 +1933,9 @@ class Interpreter(InterpreterBase): if not dep: raise exception - self.coredata.deps[identifier] = dep + # Only store found-deps in the cache + if dep.found(): + self.coredata.deps[identifier] = dep return DependencyHolder(dep) def get_subproject_infos(self, kwargs): -- cgit v1.1