Changeset View
Standalone View
swh/loader/mercurial/from_disk.py
Show First 20 Lines • Show All 161 Lines • ▼ Show 20 Lines | ): | ||||
# hg node id of the latest snapshot branch heads | # hg node id of the latest snapshot branch heads | ||||
# used to find what are the new revisions since last snapshot | # used to find what are the new revisions since last snapshot | ||||
self._latest_heads: List[bytes] = [] | self._latest_heads: List[bytes] = [] | ||||
self._load_status = "eventful" | self._load_status = "eventful" | ||||
# If set, will override the default value | # If set, will override the default value | ||||
self._visit_status = None | self._visit_status = None | ||||
self.old_environ = os.environ.copy() | |||||
os.environ.clear() | |||||
os.environ.update(get_minimum_env()) | |||||
def pre_cleanup(self) -> None: | def pre_cleanup(self) -> None: | ||||
"""As a first step, will try and check for dangling data to cleanup. | """As a first step, will try and check for dangling data to cleanup. | ||||
ardumont: do they have a clean environment because we clean it for the tests already?
(if yes, have you… | |||||
Not Done Inline Actions
ping? My trail of thought there is why bypass this in test [1] and not remove the cleaning up done in the test context altogether instead. [1] opening divergent behavior for test in production code sounds wrong to me. ardumont: > do they have a clean environment because we clean it for the tests already?
> (if yes, have… | |||||
Done Inline ActionsSorry, I must have misunderstood part of your comment and thought only the code change would suffice. I've figured out the underlying issue. I had a loader that I was initializing without calling .load(), which caused the environment to be left in a broken state. I'm updating this patch to instead do this in pre_cleanup. The full explanation is in the commit message. Alphare: Sorry, I must have misunderstood part of your comment and thought only the code change would… | |||||
Not Done Inline Actions\o/ ardumont: \o/ | |||||
This should do its best to avoid raising issues. | This should do its best to avoid raising issues. | ||||
""" | """ | ||||
clean_dangling_folders( | clean_dangling_folders( | ||||
self._temp_directory, | self._temp_directory, | ||||
pattern_check=TEMPORARY_DIR_PREFIX_PATTERN, | pattern_check=TEMPORARY_DIR_PREFIX_PATTERN, | ||||
log=self.log, | log=self.log, | ||||
) | ) | ||||
self.old_environ = os.environ.copy() | |||||
os.environ.clear() | |||||
os.environ.update(get_minimum_env()) | |||||
def cleanup(self) -> None: | def cleanup(self) -> None: | ||||
"""Last step executed by the loader.""" | """Last step executed by the loader.""" | ||||
os.environ.clear() | os.environ.clear() | ||||
os.environ.update(self.old_environ) | os.environ.update(self.old_environ) | ||||
if self._repo_directory and os.path.exists(self._repo_directory): | if self._repo_directory and os.path.exists(self._repo_directory): | ||||
self.log.debug(f"Cleanup up repository {self._repo_directory}") | self.log.debug(f"Cleanup up repository {self._repo_directory}") | ||||
rmtree(self._repo_directory) | rmtree(self._repo_directory) | ||||
▲ Show 20 Lines • Show All 484 Lines • Show Last 20 Lines |
do they have a clean environment because we clean it for the tests already?
(if yes, have you tried to remove that setup ^and check the behavior without the conditional?)
If we absolutely need this, at least, centralize the conditional in a function:
but if you prefer returning the conditional (make it negative in the function, positive at call time), fine with me as well ;)