-
Notifications
You must be signed in to change notification settings - Fork 38
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: expand ~ to user home dir #557
base: mainline
Are you sure you want to change the base?
Conversation
1931b24
to
8f3459c
Compare
8f3459c
to
12de035
Compare
@@ -1120,7 +1120,7 @@ def set_root_path(self, original_root: str, new_root: str) -> None: | |||
(It will store the new root path as an absolute path.) | |||
""" | |||
# Need to use absolute to not resolve symlinks, but need normpath to get rid of relative paths, i.e. '..' | |||
new_root = str(os.path.normpath(Path(new_root).absolute())) | |||
new_root = str(os.path.normpath(Path(new_root).expanduser().absolute())) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This place will call expanduser()
on all calls to set_root_path
, but I doubt that's what we want to do. The original issue is about interactive user input, I think it's better to put this call immediately after that input is collected instead of inside this setter.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we do want it in here since we make a call to pathlib.Path.absolute()
which will prepend the current working directory to the path in cases where the path starts with ~
, resulting in a root path like /home/jericht/~/tmp/123
, which is something we don't want in my opinion.
We are applying os.normpath
and pathlib.Path.absolute
to the path, so we're already not setting the new_root
parameter as is to the self.outputs_by_root
dict. To me, this means the code is intended to "make sure the path is a valid root path". However, transforming a path like ~/tmp/123
into something like /the/current/working/dir/~/tmp/123
, for example, seems like a bug. If the caller really wants to set a root path like /the/current/working/dir/~/tmp/123
by just passing in ~/tmp/123
while the cwd is /the/current/working/dir/
, I think it's reasonable to have the caller pass in the absolute path in this case.
I've updated the function docstring to make it clear that this happens along with the other transformations it applies to new_root
.
I tested this locally to make sure expanduser
handles some edge cases I could think of correctly. If the path is already has the ~
expanded, it won't attempt to expand it again creating an invalid path. Here are a few tests I ran locally.
>>> def test_expanduser(path: str):
... print(f"os.path.expanduser: {os.path.expanduser(path)}")
... print(f"pathlib.Path.expanduser: {pathlib.Path(path).expanduser()}")
...
>>> test_expanduser("~/tmp/123")
os.path.expanduser: /home/jericht/tmp/123
pathlib.Path.expanduser: /home/jericht/tmp/123
>>> test_expanduser("~/~/tmp/123")
os.path.expanduser: /home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/~/tmp/123
>>> test_expanduser("/tmp/~/123")
os.path.expanduser: /tmp/~/123
pathlib.Path.expanduser: /tmp/~/123
>>> test_expanduser("/home/jericht/~/tmp/123")
os.path.expanduser: /home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/~/tmp/123
>>> test_expanduser("~/home/jericht/~/tmp/123")
os.path.expanduser: /home/jericht/home/jericht/~/tmp/123
pathlib.Path.expanduser: /home/jericht/home/jericht/~/tmp/123
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
side comment: Does this change also work for deadline attachment download
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If we do want this expansion to happen always in the setter, I think we should look more closely at where the value comes from currently, and where it could come from. Will this be surprising in any case?
Signed-off-by: Jericho Tolentino <[email protected]>
12de035
to
2a4aead
Compare
Quality Gate passedIssues Measures |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does this change also affect deadline attachment download
? I hope the same logic (or the same code path) will apply
|
Fixes: #549
What was the problem/requirement? (What/Why)
When running
deadline job download-output
on a machine that has a different root path format than the machine a job was submitted from, the CLI prompts the user to enter a new root path to download output files to. If you use a~
, it does not properly expand that to the user home directory.What was the solution? (How)
Expand
~
What is the impact of this change?
CLI prompt now expands
~
to user home directoryHow was this change tested?
See DEVELOPMENT.md for information on running tests.
download
orasset_sync
modules? If so, then it is highly recommendedthat you ensure that the docker-based unit tests pass.
Manual testing on Mac. Verified it expands the
~
correctly:Was this change documented?
No
Does this PR introduce new dependencies?
This library is designed to be integrated into third-party applications that have bespoke and customized deployment environments. Adding dependencies will increase the chance of library version conflicts and incompatabilities. Please evaluate the addition of new dependencies. See the Dependencies section of DEVELOPMENT.md for more details.
Is this a breaking change?
No
Does this change impact security?
No
By submitting this pull request, I confirm that you can use, modify, copy, and redistribute this contribution, under the terms of your choice.