Skip to content
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

Add new use_canonical_path option in rezconfig.py #1543

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

OlivierArgentieri
Copy link

@OlivierArgentieri OlivierArgentieri commented Sep 30, 2023

Context:

It's because on windows, we can map some drive letter, to UNC network path.
To centralize packages but also software binaries.

Problem:

Some softwares like substance, vscode or RV, cannot be run on UNC path and will crash or freeze.

Solution:

Map a new drive letter on windows, using new-smbmapping for example, to avoid usage of unc path in PATH environment variable.
But, rez resolution plugin, use os.path.realpath directly in __init__ because of that, the path is converted back from our mapped drive letter to full unc path, so some binaries cannot be launched anymore.

Another solution, can be the usage of rez cache, but i found this solution not efficient rather than just execution of binary from network.

Final details:

It's why, i have created a new parameter in the rezconfig, to have an option to bypass the convertion of canonical path on resolution process, in case of mapped letter drive in packages_paths.

Obviously, by default i have set it to True, to not alterate the current behavior of rez resolution plugin.

Test:

  • I have locally reinstall rez with theses modifications, then i have set the property use_canonical_path to False in the rezconfig.py to test and here the result i got:
PS C:\Users\oargentieri> rez env vscode-1.82

You are now in a rez-configured environment.

resolved by oargentieri@<hostname> on Thu Sep 28 10:50:16 2023, using Rez v2.113.0

requested packages:
vscode-1.82
~platform==windows  (implicit)
~arch==AMD64        (implicit)
~os==windows-10.0   (implicit)

resolved packages:
platform-windows  X:\rez_package\platform\windows
vscode-1.82.2     X:\rez_package\vscode\1.82.2\platform-windows

And now vscode can be launched using rez env vscode -- code --disable-chromium-sandbox

Issue:

Also linked to this discussion

Signed-off-by: OlivierArgentieri <olivierargentieri@gmail.com>
@OlivierArgentieri OlivierArgentieri requested a review from a team as a code owner September 30, 2023 21:53
@JeanChristopheMorinPerso
Copy link
Member

Thanks! I'm far from Windows expert so I'll let others chime in on if it's something we want or not. I'm also wondering why this has never come up before...

@maxnbk
Copy link
Contributor

maxnbk commented Sep 30, 2023

I've only skimmed, so I might be missing something, but...

Is this not essentially this issue manifesting? #1438

If so, I'm wondering if we shouldn't just apply this without a config value anyway?

@OlivierArgentieri
Copy link
Author

OlivierArgentieri commented Sep 30, 2023

I've only skimmed, so I might be missing something, but...

Is this not essentially this issue manifesting? #1438

If so, I'm wondering if we shouldn't just apply this without a config value anyway?

Actually yes I think it's related to #1438 , maybe the behavior of os.path.realpath has changed.

Introducing a new variable in config, will allow us to keep the behavior as it is, for example, on Linux, and to disable the canonicals path resolution on Windows or if an unc path are in packages_paths and match a mapped letter drive on the local computer.

But yes, maybe we could actually fix this in another way.

Signed-off-by: Olivier Argentieri <olivierargentieri@gmail.com>
@OlivierArgentieri
Copy link
Author

Hi !

Do you have any update on this ?
Because this issue still exist in the new version of rez (tested with 3.0.0)

Thank you !

@maxnbk
Copy link
Contributor

maxnbk commented Feb 22, 2024

I think based on #1438 we should try to see if the solution proposed in TSC at the time is the correct step to take. I would prefer a configration-less solution if possible. I am going to start tinkering with this issue to see the effects of replacing the realpath calls and try to start to figure out what sort of testing might be needed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants