-
Notifications
You must be signed in to change notification settings - Fork 337
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Fix pkg cache test #1046
Fix pkg cache test #1046
Conversation
Actually, would it be better if we make path canonical right after it's loaded here ? Like : @cached_property
def _data(self):
data = copy.deepcopy(self._data_without_overrides)
# need to do this regardless of overrides, in order to flatten
# ModifyList instances
deep_update(data, self.overrides)
if data["default_cachable_per_repository"]:
canonised = dict()
for path, value in data["default_cachable_per_repository"].items():
canonised[canonical_path(path)] = value
data["default_cachable_per_repository"] = canonised
return data And maybe we could also do this for other filesystem path related config settings ? |
Sorry, I think for now I'll just focusing on fixing tests. |
The last commit is for fixing As an example, see part of my test report below :
|
# Conflicts: # src/rez/tests/test_commands.py # src/rez/tests/test_package_cache.py
Hey David, I'll merge this but I think we're bandaiding the real issue somewhat. I agree that we should be converting to canonical paths at config load time (well, during its lazy validation anyway) as you've suggested. However you wouldn't do it as shown in your snippet - instead you'd do it using a Setting-subclassed validator (as per examples in config.py), so you don't get the cost of canonicising paths at config load time. In terms of the issue you've found in (eg) setenv - that's trickier because you can't know whether a string being used in setenv is representing a path or not, even if it looks like one. To properly solve this I think we'd have to introduce another built in construct, for eg something like:
|
Ah yes, that sounds much better !
That sounds good too, will have a look into that. :) |
Package.is_cachable
may not return correct setting whenconfig.default_cachable_per_repository
is set and the paths it contains is not canonical, like when filesystem is slash/case-insensitive (Windows).TestPackageCache.test_caching_on_resolve
can only get passed in production install due toResolvedContext
will not trigger cache update if that is not the case.