-
Notifications
You must be signed in to change notification settings - Fork 90
Anatomy: expand user and vars in root paths #1884
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
base: develop
Are you sure you want to change the base?
Changes from 8 commits
bcb2c1c
d2a7d2f
9a2e29d
2a9d64f
85ca603
8510253
9f79e1a
c49633b
3bb45c7
319a2c6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,3 +1,4 @@ | ||
| from __future__ import annotations | ||
| import os | ||
| import platform | ||
| import numbers | ||
|
|
@@ -42,17 +43,24 @@ def __init__(self, parent, root_raw_data, name): | |
| # as production safe. Some features may not work as expected, for | ||
| # example USD resolver or site sync. | ||
| try: | ||
| self.value = lowered_platform_keys[current_platform].format_map( | ||
| os.environ | ||
| ) | ||
| except KeyError: | ||
| self.value = ( | ||
| os.path.expandvars( | ||
| os.path.expanduser( | ||
| lowered_platform_keys[current_platform]))) | ||
| except AttributeError as e: | ||
| msg = f"Missing root definition for platform {current_platform}." | ||
| raise RootMissingEnv(msg) from e | ||
|
|
||
| try: | ||
| self.value = self.value.format_map(os.environ) | ||
| except KeyError as e: | ||
| result = StringTemplate(self.value).format(os.environ.copy()) | ||
| is_are = "is" if len(result.missing_keys) == 1 else "are" | ||
|
antirotor marked this conversation as resolved.
|
||
| missing_keys = ", ".join(result.missing_keys) | ||
| raise RootMissingEnv( | ||
| f"Root \"{name}\" requires environment variable/s" | ||
| f" {missing_keys} which {is_are} not available." | ||
| ) | ||
| ) from e | ||
|
|
||
| self.clean_value = self._clean_root(self.value) | ||
|
|
||
|
|
@@ -197,6 +205,13 @@ def find_root_template_from_path(self, path): | |
|
|
||
| All platform values are checked for this replacement. | ||
|
|
||
| Both the input ``path`` and the stored root values are tried in their | ||
| original *and* expanded forms (via ``os.path.expandvars`` / | ||
| ``os.path.expanduser``) so that the following mismatches are handled: | ||
|
|
||
| - ``path`` is already expanded but the stored root still contains | ||
| ``~` / ``$VAR`` tokens (or vice-versa). | ||
|
|
||
| Args: | ||
| path (str): Path where root value should be found. | ||
|
|
||
|
|
@@ -225,22 +240,65 @@ def find_root_template_from_path(self, path): | |
| output = str(path) | ||
|
|
||
| mod_path = self._clean_path(path) | ||
| # Expanded version of the input path – used when the stored root value | ||
| # is already expanded while the caller passed an unexpanded path, or | ||
| # to normalise both sides consistently. | ||
| expanded_mod_path = self._clean_path( | ||
| os.path.expandvars(os.path.expanduser(path)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should not allow to pass in not expanded path.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. why? shouldn't one place take care of that intead reimplementing the logic in all places where you want to use this feature before calling
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If you call
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. that is what you get now: ar = AnatomyRoot(
parent=...,
root_raw_data={"linux": "/home/user/projects"},
name="work",
)
success, result = item.find_root_template_from_path("~/projects/shot/file.ma")you'll get it handles matching unexpanded paths to expanded roots in various forms (
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. And where you'd get the path |
||
| ) | ||
|
|
||
| for root_os, root_path in self.cleaned_data.items(): | ||
| # Skip empty paths | ||
| if not root_path: | ||
| continue | ||
|
|
||
| _mod_path = mod_path # reset to original cleaned value | ||
| # Expand variables in the stored root path so we can compare it | ||
| # against an already-expanded input path (and vice-versa). | ||
| expanded_root_path = self._clean_root( | ||
| os.path.expandvars(os.path.expanduser(root_path)) | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Root paths should be already expanded in
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I am not sure I get it - it is not expanded there, only for current platform. so are you suggesting that the expansion logic should be moved to ... probably |
||
| ) | ||
|
|
||
| _mod_path = mod_path | ||
| _expanded_mod_path = expanded_mod_path | ||
| _root_path = root_path | ||
| _expanded_root_path = expanded_root_path | ||
| if root_os == "windows": | ||
| root_path = root_path.lower() | ||
| _mod_path = _mod_path.lower() | ||
| _expanded_mod_path = _expanded_mod_path.lower() | ||
| _root_path = _root_path.lower() | ||
| _expanded_root_path = _expanded_root_path.lower() | ||
|
|
||
| replacement = "{" + self.full_key + "}" | ||
|
|
||
| if _mod_path.startswith(root_path): | ||
| # 1) original path vs original root (existing behaviour) | ||
| if _mod_path.startswith(_root_path): | ||
| result = True | ||
| replacement = "{" + self.full_key + "}" | ||
| output = replacement + mod_path[len(root_path):] | ||
| break | ||
|
|
||
| # 2) original path vs expanded root | ||
| # (root stored with vars, path already expanded) | ||
| if _mod_path.startswith(_expanded_root_path): | ||
| result = True | ||
| output = replacement + mod_path[len(expanded_root_path):] | ||
| break | ||
|
|
||
| # 3) expanded path vs original root | ||
| # (path stored with vars, root already expanded) | ||
| if _expanded_mod_path.startswith(_root_path): | ||
| result = True | ||
| output = replacement + expanded_mod_path[len(root_path):] | ||
| break | ||
|
|
||
| # 4) expanded path vs expanded root (both sides have vars) | ||
| if _expanded_mod_path.startswith(_expanded_root_path): | ||
| result = True | ||
| output = ( | ||
| replacement | ||
| + expanded_mod_path[len(expanded_root_path):] | ||
| ) | ||
| break | ||
|
|
||
| return (result, output) | ||
|
|
||
|
|
||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.
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 gona do it, I'm against using
expandvars, only{ENV}should be possible.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.
what is the reason? My argument is that using paths with
$VARinside is pretty standard.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.
It is standard, but then you have to support it at every place which is using roots, including USD resolver, it is hard to find out if it is missing, and what exact variable it is (it does not have strictly defined end the the variable name).
$MY_custom_varis valid env key, but how you find out if it should be$MYor$MY_custom...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 guess the behavior is clearly defined.
VAR = "foo":lorem $VAR:lorem foolorem $VARipsum:loremlorem $VAR ipsum:lorem foo ipsumlorem %VAR%:lorem foolorem %VAR%ipsum:lorem fooipsumos.path.expandvars()is taking care of that in python and the resolver needs to implement{VAR}format anyway so adding$VARand/or%VAR%is trivial at that point. But I don't really mind. @BigRoy ?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.
You can't easily find out if it is missing (without validating it for each platform) and there are limitations, e.g. you can't use
$VARipsum(which is missing in your example). I'm 100% against it. I know it is "known", but it is not easily detectable and easy to validate.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.
It is there:
lorem $VARipsum:loremBut it also supports curly bracket format that is standard too. So if you want this to work, you can:
lorem ${VAR}ipsum:lorem fooipsumand all that is supported by
os.path.expandvars()Uh oh!
There was an error while loading. Please reload this page.
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 is unresolved expanding and you have no idea it is unresolved
This does require to be more specific, something that should be filled is not
Supporting this will only bring issues. At the end it does what the python formatting does, but with less control and more headaches for us.