bug fix: migrations do not respect core.cache:storage_path #15130
Conversation
|
|
smoofra
force-pushed
the
cache-migration
branch
2 times, most recently
from
ef1a432 to
fd42bfc
Compare
There was a problem hiding this comment.
Hi @smoofra
Thanks very much for this contribution!
I think there are 2 different things here:
- The fix of the bug that is ignoring the
global.confstoragedefinition - A new system for migrations that is able to migrate the "storage", assigning a version to the storage too.
I think we should decouple both, first, fix quickly the bug, so migration happens even if storage conf is defined, assuming the current 1:1 assumption between the Conan home and the package storage.
Then, we should consider more carefully the change of migration approach with versions in the storage folder, because this can have some challenges, better handle them separately.
smoofra
force-pushed
the
cache-migration
branch
from
fd42bfc to
c729ad6
Compare
OK, how's it look now? |
migrations do not respect core.cache:storage_path. If the user has this option set, and tries to migrate from conan 2.0.13 to 2.0.14, the migration will fail. This fix still assumes a 1-1 relationship between the $CONAN_HOME and the local cache, but the forward and backward migrations are updated to find the local cache in the correct way.
smoofra
force-pushed
the
cache-migration
branch
from
c729ad6 to
65e0894
Compare
There was a problem hiding this comment.
I have proposed #15135 as an alternative for this PR.
Thanks very much for your contribution, it is technically good, but there some architecture aspects that we would like to do slightly different:
- We prefer all the subapis to have the same interface for construction
- The LRU DB migration is a temporary one, once the 2.X train starts (2.1, 2.2...) can be removed later, so it is better that it doesn't affect other parts of the code
- So a little bit of repetition like the one that happens in the migration can be preferred
I have also included a test in my PR to check that the problem is really addressed.
| @@ -49,7 +49,7 @@ def __init__(self, cache_folder=None): | |||
| self.graph = GraphAPI(self) | |||
| self.export = ExportAPI(self) | |||
| self.remove = RemoveAPI(self) | |||
| self.config = ConfigAPI(self) | |||
| self.config = ConfigAPI(self.home_folder) | |||
There was a problem hiding this comment.
Better not change the interface of ConfigAPI constructor. All subapis are expected to receive the ConanAPI as argument.
| def __init__(self, conan_api): | ||
| self.conan_api = conan_api | ||
| def __init__(self, home_folder): | ||
| self.home_folder = home_folder |
There was a problem hiding this comment.
All changes in this file seems unnecessary, just a side effect of changing the ConfigAPI constructor. Better do not change this file.
ok 😃 but I think you should still include a fixed version of the undo script so that migration backwards to 2.13, and then forwards to 2.15 still works |
migrations do not respect core.cache:storage_path. If the user has this option set, and tries to migrate from conan 2.0.13 to 2.0.14, the migration will fail.
To fix, client migrations are split into two migrators, one for $CONAN_HOME, and one for the local cache.
The local cache now has its own version.txt and migrations/ directory,
for future migrations. But the undo script from 2.0.14 is still
placed in $CONAN_HOME, because existing versions of conan will
not know to look for it anywhere else.
The undo script from 2.0.14 is also updated to use the config to find the correct path to the local cache.
fixes #15129