Relative paths resolved without changing the working directory - #979
Merged
Merged
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #979 +/- ##
=========================================
Coverage 100.00% 100.00%
=========================================
Files 27 27
Lines 9079 9077 -2
=========================================
- Hits 9079 9077 -2 ☔ View full report in Codecov by Harness. |
This comment was marked as outdated.
This comment was marked as outdated.
- Remove the sys.path modification from path_dir_context. It races through process-global state, and a class_path naming a module next to the config file was never a documented feature. - Don't mask a parse error with a FileNotFoundError when the working directory has been removed, by falling back to the absolute path in _describe_origin. - Anchor the jsonnet snippet name on the base name, so that from_config resolves imports for a config given as a relative path with a directory. - Skip test_relative_path_context_cwd_removed in windows, where the working directory can't be removed. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
|
This file contains hidden or 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
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.



What does this PR do?
Relative paths in a config file are resolved with respect to the directory of that file. This was done by temporarily changing the process working directory with
os.chdir, which is process-global state, with several consequences, most visible for configs in temporary directories:FileNotFoundError.os.getcwd()resolves symlinks, so a config under a symlinked directory gave back paths the user never wrote.Changes:
_paths.py:change_to_path_diris renamed topath_dir_context, since it no longer changes any directory. It sets the newcurrent_local_dircontext variable, whichPath.__init__uses for local relative paths. Keeping it separate from_current_path_dirmeans a local path inside a remote config still resolves against the closest local directory._formatters.py:get_initial_working_directoryis removed, the working directory is now always the initial one._describe_originfalls back to the absolute path when the working directory is gone, so that a removed directory does not mask the parse error being reported._loaders_dumpers.py: jsonnet resolvesimportstatements relative to the file name given toevaluate_snippet, previously found through the working directory, now anchored oncurrent_local_dir. Its base name is used, which also fixesfrom_configfor a config given as a relative path with a directory, e.g.sub/config.jsonnet, broken the same way before this change.typing.py: a path dumped withpath_dump_preserve_relativeis reloaded by passing its storedcwdto thePathconstructor, instead of the context manager, which required that directory to exist.Code that relied on the working directory, e.g. a custom type that opens a relative path, must now use a path type such as
Path_fr. Aclass_pathnaming a module next to the config file also stops working, since that resolved through the working directory; it was never a documented feature, so it can be added back on request. The migration guide lists this as breaking.Not addressed: a working directory removed before parsing starts still raises
FileNotFoundError, even for an absolute config path.Before submitting