Fix shebang and executable paths in gitbash #1364

amorphousWaste · 2022-08-20T07:57:59Z

Originally, I thought "shebang and executable paths incorrect in gitbash" was the cause of issues when working with rez in gitbash/Windows, however after investigation and discussion, there were much deeper issues. This PR addresses issues with pathing by adding multiple options for pathing to the needed shells and executor classes.

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

This is for working with non-posix paths. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Allows for working with Windows drive directories like posix root paths. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Fixes issues with spaces in names. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

JeanChristopheMorinPerso · 2022-08-20T18:49:38Z

@amorphousWaste we will be happy to get the fixes for the linter, but I think it would be more appropriate if it was done in a separate PR.

maxnbk · 2022-08-20T18:54:04Z

linting fixes can be placed against this issue.
#1356

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste · 2022-08-21T08:01:06Z

My apologies. I misunderstood the workflow checks and thought that linting was blocking the PR. It has been removed.

nerdvegas · 2022-09-12T11:02:59Z

For visibility please see recent comments: #1302
Would be good to clear this one up, it's been kicking on for a bit.

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add check and key conversion for shell pathed key based on arg Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

JeanChristopheMorinPerso · 2022-09-23T00:24:37Z

@amorphousWaste can you update the PR description please to give us an overview of how all this is used, describe what has been tested, potential gray areas, areas not covered, etc please?

That would be greatly appreciated!

amorphousWaste · 2022-09-23T05:19:36Z

This update to the PR adds the functionality of choosing whether or not to modify paths and shell paths via the rezconfig.py on a per-shell and per-variable basis. The implementation is delegated to the shell plugin so that how the paths are changed or not is up to the shell. This will apply to environment building and package building. This should conform the previous fixes to fit the points brought up in the various conversations. All previous tests pass in CMD, PowerShell, and Gitbash on Windows. Gitbash has been a struggle to implement and further grey areas are unknown. Hopefully, this will give Gitbash the same functionality as existing shells with no further caveats.

src/rez/utils/sourcecode.py

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

JeanChristopheMorinPerso · 2022-11-17T22:09:01Z

Thanks @amorphousWaste for walking us through your PR during the TSC meeting. Here are the points we would like to see be implemented before we can merge this PR:

Add a feature flag so that we can have the feature disabled by default. This would be off for a while until we are fully confident that things are working as expected. And it would also help when troubleshooting issues.
Add logs that will tell us which environment variable were normalized and the paths.
Add comments in the shell scripts for each normalized paths (or environment variables) affected.
Add tests.

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add path conversion info into file. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

JeanChristopheMorinPerso · 2022-12-08T23:34:04Z

Hi @amorphousWaste , I see you pushed some new commits to the PR. What's the state of things right now? Can see start reviewing or there is still more work to do before it's ready to review?

Signed-off-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>

maxnbk · 2023-02-12T17:20:58Z

Just want to write that during the last TSC meeting (january) that @amorphousWaste mentioned that the PR is ready for review and testing by interested parties.

@JeanChristopheMorinPerso registered interest in reviewing, and @instinct-vfx registered willingness to try throwing it in front of a few tester individuals in a studio environment, though I imagine that is subject to the whims and priorities of the studio.

JeanChristopheMorinPerso · 2023-02-12T18:33:42Z

@maxnbk I am actively reviewing this PR. I've been doing for a couple of hours now. I should be done today.

JeanChristopheMorinPerso

Thanks for all your work on this @amorphousWaste! I did a first pass on this. There is some things to fix, but the other things are mostly questions, comments, notes, etc.

Note that I haven't tested it yet.

Apart from that, we requested to add some tests. I see some tests were added, but they are pretty light. The path normalization functions are not well covered, corner cases aren't unit tested, etc. I'd also love to see real integration tests, i.e. have packages that we build, resolve in multiple shells and verify that everything looks like it should work works as expected.

Lastly, it's missing documentation. I believe @jasoncscott volunteered to help on that? I would see a new section of our wiki dedicated to that, with example, explanations, etc.

JeanChristopheMorinPerso · 2023-02-12T16:38:19Z

src/rez/rezconfig.py

+# as a result of both the shell and the settings in "pathed_env_vars" and
+# "shell_pathed_env_vars". This is meant to aid in debugging and should be
+# False unless needed.
+disable_normalization = False


I would call it disable_path_normalization, also it should be set to True so that it's disabled by default.

JeanChristopheMorinPerso · 2023-02-12T16:43:21Z

src/rezplugins/shell/cmd.py

@@ -261,6 +261,10 @@ def as_path(self, path):
        Returns:
            (str): Transformed file path.
        """
+        # Prevent path conversion if normalization is disabled in the config.
+        if config.disable_normalization:


Could this be done in the normalize_path path instead?

JeanChristopheMorinPerso · 2023-02-12T16:48:16Z

src/rezplugins/shell/_utils/powershell_base.py

+            converted_path = convert_path(path, 'windows')
+            if path != converted_path:
+                print_debug(
+                    'Path converted: {} -> {}'.format(path, converted_path)


How about Path normalized? Also, both converted and changed are used. I think we should make sure the message is consistent. Lastly, I'd use {!r} to automatically quote the paths. This usually helps with debugging whitespace issues, etc.

JeanChristopheMorinPerso · 2023-02-12T16:53:50Z

src/rezplugins/shell/_utils/windows.py


-    return path
+    if path != new_path:
+        print_debug('Path converted: {} -> {}'.format(path, new_path))


Won't this cause duplicate logs? Should the responsibility to log be of the caller or the callee? In general the responsibility is delegated to the caller, but we could say that in this case it's the callee.

JeanChristopheMorinPerso · 2023-02-12T16:54:25Z

src/rezplugins/shell/cmd.py

+
+        if path != converted_path:
+            self._addline(
+                'REM Path converted: {} -> {}'.format(


Again, converted vs changed.

JeanChristopheMorinPerso · 2023-02-12T17:38:16Z

src/rezplugins/shell/gitbash.py

-"""
-Git Bash (for Windows) shell
-"""
+"""Git Bash (for Windows) shell."""


Small note: Please avoid unnecessary formatting changes.

JeanChristopheMorinPerso · 2023-02-12T17:39:34Z

src/rezplugins/shell/gitbash.py


-        return super(GitBash, self).normalize_paths(value2)
+    def shebang(self):
+        self._addline('#! /usr/bin/env bash')


Suggested change

self._addline('#! /usr/bin/env bash')

self._addline('#!/usr/bin/env bash')

JeanChristopheMorinPerso · 2023-02-12T17:45:44Z

src/rez/tests/test_shells.py

+        config.override('disable_normalization', True)
+
+        test_path = r'C:\foo\bar\spam'
+        normalized_path = shell.normalize_path(test_path)


shell is a string, which cause the test to fail with

AttributeError: 'str' object has no attribute 'normalize_path' (in shell 'bash')

JeanChristopheMorinPerso · 2023-02-12T17:49:21Z

src/rez/rex.py

+        if shell_name not in config.shell_pathed_env_vars:
+            return False
+
+        return any(


This is not covered by any tests

JeanChristopheMorinPerso · 2023-02-12T17:51:15Z

src/rez/rex.py

@@ -1359,7 +1422,7 @@ def normalize_path(self, path):
        Returns:
            str: The normalized path.
        """
-        return self.interpreter.normalize_path(path)
+        return self.interpreter.as_path(path)


This is not covered by any tests. It could be a good occasion to add coverage here.

JeanChristopheMorinPerso · 2023-03-03T22:01:45Z

Hey @amorphousWaste @Jawabiscuit , would one of you be willing and have time to pick this up again? If not, please let us know so that we can see if we can find someone else that might be interested.

This PR is important for our community, so we would like to get things moving.

Jawabiscuit · 2023-03-04T00:12:22Z

@JeanChristopheMorinPerso yeah, definitely trying to carve out some time in the near future to revisit this.

I'm wrapping up an extended effort for Linux/Windows Deadline rendering, which also incorporates rez. I'm better positioned now to take another look and continue with the converting to rez on Windows effort soon.

I've enabled gitbash on Windows for myself at least since the last meeting to verify our experience and it has been stable for me working as a developer. Beginning next week I should be able to get more than just me testing and take a look at the comments on this PR in more detail.

I defer to @amorphousWaste on what his plans are, but happy to jump in and take it home if he's preoccupied.

JeanChristopheMorinPerso · 2023-03-04T00:16:52Z

Great news @Jawabiscuit! Just let us know how we can help and of your plans change.

predat · 2023-03-05T12:42:52Z

If I may put my two cents in, i was able to build zlib library with cmake & msvc like i do on linux with a CMakeLists.txt with this PR.

jasoncscott · 2023-03-07T09:34:42Z

src/rez/tests/test_shell_utils.py

+    def test_path_conversion_mixed(self):
+        """Test the path conversion to mixed style."""
+        test_path = r'C:\foo\bar\spam'
+        converted_path = convert_path(test_path, 'unix')


Isn't this the same test as the previous one? Should it be testing mixed?

JeanChristopheMorinPerso · 2023-09-09T17:27:11Z

Closing in favor of #1475.

amorphousWaste added 9 commits August 20, 2022 00:54

Add pathing options to shells

253702f

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add pathing options for cmd

666d55f

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add pathing options to gitbash

3d2e429

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add slash expansion

7fd92e7

This is for working with non-posix paths. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Conform drive start letters

cfafdbe

Allows for working with Windows drive directories like posix root paths. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Quote executable

9c7caa0

Fixes issues with spaces in names. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Use new pathing option for shell paths

6bf148b

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add pathing options to rex

956ecb3

With these options, complex shell implementation can choose to use differently converted paths for different reasons. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Use new path option

ab0bdab

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste requested a review from nerdvegas as a code owner August 20, 2022 07:58

amorphousWaste mentioned this pull request Aug 20, 2022

shebang and executable paths incorrect in gitbash #1302

Open

amorphousWaste force-pushed the fixes_#1302 branch from a720b69 to bab60ce Compare August 20, 2022 18:04

JeanChristopheMorinPerso changed the title ~~Fixes #1302~~ Fix shebang and executable paths in gitbash Aug 20, 2022

Remove linting fixes

d1b071d

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste force-pushed the fixes_#1302 branch from bab60ce to d1b071d Compare August 21, 2022 08:00

amorphousWaste requested a review from a team as a code owner September 22, 2022 23:02

amorphousWaste added 6 commits September 22, 2022 16:04

Add shell_pathed_env_vars to config

4376a38

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add classmethod to check for shell pathed keys

24dbc7c

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add is_shell_path arg to escape_string

9c00baf

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add is_shell_path arg to escape_string overrides

3677b00

Add check and key conversion for shell pathed key based on arg Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Update call to escape_string with is_shell_path arg

57123b1

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Capitalize drive letter on windows path conversion

8aae074

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste force-pushed the fixes_#1302 branch from 848c894 to 8aae074 Compare September 22, 2022 23:05

Jawabiscuit reviewed Sep 24, 2022

View reviewed changes

src/rez/utils/sourcecode.py Outdated Show resolved Hide resolved

amorphousWaste added 2 commits October 25, 2022 12:46

Fix path conversions

78abeab

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add special logic for implicit keys and values

2883f73

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste force-pushed the fixes_#1302 branch from 7d6273c to 2883f73 Compare October 25, 2022 19:46

JeanChristopheMorinPerso mentioned this pull request Nov 16, 2022

TSC Meeting 2022-11-17 #1408

Closed

JeanChristopheMorinPerso removed request for a team and nerdvegas November 17, 2022 23:36

JeanChristopheMorinPerso added the os:windows Windows-specific label Nov 17, 2022

amorphousWaste added 6 commits November 24, 2022 11:12

Merge with 2.112.0 changes

c3caada

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add disable_normalization flag to config

8dfde24

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add disable normalization test

e81756c

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Create shell utils tests

efc95f4

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add logic for skipping path normalization

8844f69

Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

Add debugging output to path conversion

6a4edb1

Add path conversion info into file. Signed-off-by: amorphousWaste <20346603+amorphousWaste@users.noreply.github.com>

amorphousWaste force-pushed the fixes_#1302 branch from 049f1d1 to 6a4edb1 Compare November 24, 2022 19:12

Merge branch 'master' into fixes_#1302

ecdb3cb

Signed-off-by: Jean-Christophe Morin <38703886+JeanChristopheMorinPerso@users.noreply.github.com>

JeanChristopheMorinPerso requested changes Feb 12, 2023

View reviewed changes

JeanChristopheMorinPerso marked this pull request as ready for review February 12, 2023 18:59

JeanChristopheMorinPerso requested a review from a team as a code owner February 12, 2023 18:59

jasoncscott reviewed Mar 7, 2023

View reviewed changes

Jawabiscuit mentioned this pull request Apr 21, 2023

Windows Gitbash Support Fixes #1302 #1475

Open

39 tasks

JeanChristopheMorinPerso closed this Sep 9, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix shebang and executable paths in gitbash #1364

Fix shebang and executable paths in gitbash #1364

amorphousWaste commented Aug 20, 2022 •

edited by JeanChristopheMorinPerso

Loading

JeanChristopheMorinPerso commented Aug 20, 2022

maxnbk commented Aug 20, 2022

amorphousWaste commented Aug 21, 2022

nerdvegas commented Sep 12, 2022

JeanChristopheMorinPerso commented Sep 23, 2022

amorphousWaste commented Sep 23, 2022

JeanChristopheMorinPerso commented Nov 17, 2022

JeanChristopheMorinPerso commented Dec 8, 2022

maxnbk commented Feb 12, 2023

JeanChristopheMorinPerso commented Feb 12, 2023

JeanChristopheMorinPerso left a comment •

edited

Loading

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso Feb 12, 2023

JeanChristopheMorinPerso commented Mar 3, 2023

Jawabiscuit commented Mar 4, 2023

JeanChristopheMorinPerso commented Mar 4, 2023

predat commented Mar 5, 2023 •

edited

Loading

jasoncscott Mar 7, 2023

JeanChristopheMorinPerso commented Sep 9, 2023

	self._addline('#! /usr/bin/env bash')
	self._addline('#!/usr/bin/env bash')

Fix shebang and executable paths in gitbash #1364

Fix shebang and executable paths in gitbash #1364

Conversation

amorphousWaste commented Aug 20, 2022 • edited by JeanChristopheMorinPerso Loading

JeanChristopheMorinPerso commented Aug 20, 2022

maxnbk commented Aug 20, 2022

amorphousWaste commented Aug 21, 2022

nerdvegas commented Sep 12, 2022

JeanChristopheMorinPerso commented Sep 23, 2022

amorphousWaste commented Sep 23, 2022

JeanChristopheMorinPerso commented Nov 17, 2022

JeanChristopheMorinPerso commented Dec 8, 2022

maxnbk commented Feb 12, 2023

JeanChristopheMorinPerso commented Feb 12, 2023

JeanChristopheMorinPerso left a comment • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JeanChristopheMorinPerso commented Mar 3, 2023

Jawabiscuit commented Mar 4, 2023

JeanChristopheMorinPerso commented Mar 4, 2023

predat commented Mar 5, 2023 • edited Loading

Choose a reason for hiding this comment

JeanChristopheMorinPerso commented Sep 9, 2023

amorphousWaste commented Aug 20, 2022 •

edited by JeanChristopheMorinPerso

Loading

JeanChristopheMorinPerso left a comment •

edited

Loading

predat commented Mar 5, 2023 •

edited

Loading