Windows Gitbash Support Fixes #1302 #1475

Jawabiscuit · 2023-04-21T22:29:30Z

Fixes #1302 by adding gitbash support.
Fixes #1321 with path normalization changes for shells.

Fixed

Rex code compilation
Executable file paths in rez-shell* context invoking scripts
Existing issue with rez bind python 2 on Windows
Existing issue where execute_shell contexts are being executed with the wrong shell when @per_available_shell() in unittests. See testing observations
Corner case with running unittests on Windows systems with WSL installed. I recommend setting path to bash.exe in system settings for best experience.
Existing issue (and partial fix implemented by Fix shebang and executable paths in gitbash #1364) with implicit requires string expansion in setenv
Existing encoding issue when building wiki locally with Python 3.7

Changed

Adds

Observations

Testing

Observed early on putting code into rezplugins/shell like cypath.py or uncpath.py causes issues when trying to test. The unittest structure for rez needs to be extended to handle plugin tests so plugin code can be encapsulated. I will break this out into a separate discussion.
Attempted to create a test that checked more specifically for spaces in executable paths breaking shell invoking scripts--one of the apparent root causes prompting #1302--but ran into inconsistencies running local vs CI. The tests run fine locally.
- In my observation rez's backport of subprocess does not correctly capture stderr when the stdin option is used. I attempted a hack workaround using Python's builtin subprocess with the args of the return of rez's subprocess call which worked locally to catch the error but not on CI.
- Most Windows shells failing to output expected paths on e2e shell tests but on Linux pwsh is the only one exhibiting the same problem.
On Windows, Python files can be executed with or without the py extension depending on the shell. Gitbash supports execution without the extension. For this reason, rez bind, which installs hello_world in many tests, was updated to support an additional --py-script-mode via the CLI and an additional argument to bind on the API side for unittests.
Most shell tests need to override the default_shell setting when @per_available_shell() is used so I modified the decorator to set this setting automatically thereby removing the necessity to state it in most shell tests.

Experience

Overall, my opinion here, the gitbash experience on Windows feels improved. Before the recent changes to normalization it was necessary to use gitbash's builtin cygpath to convert paths in build scripts embedded into package defs. This is no longer necessary, which feels more cross-platform capable, however more complicated build scripts outside of a package.py would likely not inherit all the benefits but to what extent remains to be tested.

Testing

How to Test/Enable

I recommend using the most recent version of Git for Windows, a backlog of features and fixes was added recently.

Git for Windows 2.41.0 comes with MSYS2 runtime (Git for Windows flavor) with all these improvements based on Cygwin 3.4.6.

Install gitbash
Clone the PR branch
Enable gitbash in the settings

The minimum viable settings required to enable gitbash:

default_shell = "gitbash"
enable_path_normalization = True

Logging Settings

Turn logging on to see normalization in action by setting the appropriate value in your rez config. Messages will print to the terminal as well as in temporary context files.

# Set this in gitbash if you need to see content of temporary context files
export REZ_KEEP_TMPDIRS=1

# Set for viewing messages in gitbash
debug_shells = True

# Print debugging info for the cygpath module
debug_cygpath = False

# Print warnings and errors useful for debugging
warn_shell_startup = True

# On windows, it's nice to set this as well
color_enabled = "force"

Path Normalization Settings

These are the most notable settings related to normalization. Please see rezconfig.py for more explicit documentation on each of these settings.

pathed_env_vars = [
    "*PATH"
]

env_var_separators = {
    "CMAKE_MODULE_PATH": ";",
    "DOXYGEN_TAGFILES": " ",
}

shell_pathed_env_vars = {
    "gitbash": [
        "PYTHONPATH",
        "CMAKE_MODULE_PATH",
    ]
}

# Path-like list variables in here will override those set in `env_var_separators`
shell_env_var_separators = {
    "gitbash": {
        "PATH": ":",
        "PYTHONPATH": ";",
    }
}

Resolved Context Execution

Here's a table of expected path modifications given the defaults above.

note	var/method	shell method	path in	path out
	context.package_paths	`normalize_path`	`c:\\Users\\jdoe\\src\\...\\packages`	`/c/Users/jdoe/src/.../packages`
[1]	executor.rez_path	`normalize_path`	`c:\\Users\\jdoe\\...\\rez`	`/c/Users/jdoe/src/.../rez`
	executor.setenv.REZ_USED_PACKAGES_PATH	`normalize_path`	`c:/Users/jdoe/src/.../packages`	`/c/Users/jdoe/src/.../packages`
[2]	executor.setenv.REZ_PATH	`normalize_path`	`c:\\Users\\jdoe\\...\\rez`	`/c/Users/jdoe/src/.../rez`
[3]	resolved_package.base	`normalize_path`	`c:\\users\\jdoe\\...\\1.0.0`	`/c/users/jdoe/src/.../1.0.0`
[3]	resolved_package.root	`normalize_path`	`c:\\users\\jdoe\\...\\1.0.0`	`/c/users/jdoe/src/.../1.0.0`
	executor.setenv.REZ_SHELL_BASE	`normalize_path`	`c:\\users\\jdoe\\...\\1.0.0`	`/c/Users/jdoe/src/.../1.0.0`
	executor.setenv.REZ_SHELL_ROOT	`normalize_path`	`c:\\users\\jdoe\\...\\1.0.0`	`/c/Users/jdoe/src/.../1.0.0`
[4]	executor.globals.root	`as_path`	`c:\\users\\jdoe\\...\\1.0.0`	`c:\\users\\jdoe\\...\\1.0.0`
[5]	executor.globals.base	`normalize_path`	`c:\\users\\jdoe\\...\\1.0.0`	`/c/Users/jdoe/src/.../1.0.0`
[6] [7]	executor.append_system_paths	`normalize_paths`	`C:\\Program Files\\git\\bin`	`${PATH}:/c/Program Files/git/bin`
			`C:\\Python37`	`${PATH}:/c/Python37`
			`${PATH}:C:\\Windows`	`${PATH}:/c/Windows`
			`${PATH}:C:\\Windows\\System32`	`${PATH}:/c/Windows/System32`
			`${PATH}:/mingw64/bin`	`${PATH}:/mingw64/bin`
			`${PATH}:/usr/bin`	`${PATH}:/usr/bin`

The interpreter always normalizes path to rez
If config.rez_1_environment_variables is set
For each resolved package
{root} variant binding, no normalization
There are no code changes here, this path is still normalized i.e. posix-style in Gitbash
fnmatch: *PATH
pathsep used: :

Note that rex delegates normalization to the shell (interpreter), calling the appropriate method for the type of path or binding. All paths are normalized to POSIX-style paths, since those are expected when using Gitbash. Gitbash also understands mixed (C:/foo/bar and quoted "C:\foo\bar" paths) but the whole point of using Gitbash is to be able to use posix style paths wherever possible.

Source Code Execution

Now we investigate what we can we expect during execution of the package.py source code which is performed after the "binding and environment setup" steps above.

import os

env.PATH.append("{root}")
env.PYTHONPATH.append(os.path.join("{root}", "src"))
env.CMAKE_MODULE_PATH.append(os.path.join(this.root, "cmake"))

Given the above code block in commands() of a package definition, here's what we can expect.

var	fnmatch	pathsep	shell method	path in	path out
env.PATH.append	`PATH`	`:`	`normalize_paths`	`c:\\users\\jdoe\\src\\...\\1.0.0`	`/c/users/jdoe/src/.../1.0.0`
env.PYTHONPATH.append	`PYTHONPATH`	`;`	`as_shell_path`	`c:\\users\\jdoe\\src\\...\\1.0.0\\src`	`c:/users/jdoe/src/.../1.0.0/src`
env.CMAKE_MODULE_PATH.append	`CMAKE_MODULE_PATH`	`;`	`as_shell_path`	`c:\\users\\jdoe\\src\\...\\1.0.0\\cmake`	`c:/users/jdoe/src/.../1.0.0/cmake`

Useful to note here, PATH is overridden to use : since that is what Gitbash expects; ; is used for PYTHONPATH and CMAKE_MODULE_PATH due to python and cmake being windows native apps that don't care what shell they are running in. Also, there's nothing technically wrong with setting CMAKE_MODULE_PATH or any other path-like variable in both env_var_separators as well as shell_env_var_separators, the latter will override the former. It can be assumed that the pathsep for PATH is usually os.pathsep so that is why it is being overridden for Gitbash in this case.

Effects of Normalization for Other Shells

Briefly, environment variables can be designated as "shell pathed" via the shell_pathed_env_vars setting but they must also implement any special logic in an as_shell_path method. Also the as_shell_path method is only used during execution of the package.py source code. Further customization needs to be handled by normalize_path or normalize_path[s] methods if necessary.

As they are today, cmd and powershell do not override their as_shell_path methods to do anything "special" and normalize_path does all the work normalizing paths and converting \s into /s. This is for better interoperability with more software like cmake and, unlike Gitbash, there is no need for normalization to be any more complex than that. If for some reason normalization this way becomes an issue then enable_path_normalization can be set to False. That should be a very unlikely occurrence since Windows shells have been able to interpret paths with mixed slashes since Windows 7.

Please see rezconfig.py for a more verbose explanation of the different ways and settings rez handles path normalization. Below are some examples of what to expect when path normalization is enabled in rez.

Resolved Context Execution

shell	path from	path to
cmd powershell pwsh	`c:\\Users\\jdoe\\src\\package_a`	`c:/Users/jdoe/src/package_a`
cmd powershell pwsh	`\\\\server\\projects\\sw\\dev\\jdoe\\packages`	`//server/projects/sw/dev/jdoe/packages`

Source Code Execution

shell	var	path from	path to
cmd powershell pwsh	CMAKE_MODULE_PATH	`\\\\server\\projects\\...\\cmake;` `\\\\server\\projects\\...\\Modules`	`//server/projects/.../cmake;` `//server/projects/.../Modules`

Known Issues

MSYS_NO_PATHCONV=1 seems to have no effect when using gitbash inside a rez-env context. Using that environment variable should inhibit gitbash from converting arguments to windows-style paths. Git for Windows version <=2.41.0 and winpty tested.

MSYS_NO_PATHCONV=1 python -c "import sys; print(sys.argv)" --dir=/tmp:/bla
['-c', '--dir=C:\\Users\\jonas\\AppData\\Local\\Temp;C:\\Program Files\\Git\\bla']

Questions

Should there be additional wiki content added and where?

Related PRs

#1364

🕶 Recommended reading

Automatic Unix -> Windows Path Conversion
Blog with a very thorough explanation of all the scenarios path conversion can be controlled - https://www.pascallandau.com/blog/setting-up-git-bash-mingw-msys2-on-windows/#the-path-conversion-issue
git-for-windows/msys2-runtime, path.h & path.cc referenced in making of this PR

linux-foundation-easycla · 2023-04-21T22:29:39Z

The committers listed above are authorized under a signed CLA.

✅ login: amorphousWaste (9f0d5ee, bfc99e2, 8eb7f5a, 7d66f00, 1e1d4b2, ff7a11c, 0ce60ab, a91a4c5, f3e2b68, 744e8f9, 7aaaecb, 640e44c, c55ee1b, a9d4de5, 7057632, 2a51e0c, 7f20c9e, e37e7f2, bb3bb0d, 51d9af0, e997c9e, f9607b5, 6834fb2, 1e5b3f2, 4976105, e12ef77, af69b8d, 489b7bc, 0b5594c, c7e7454, 42e37f9, 267c3e3, 9d0bdde, db30108, 688337a, 3b6b3c5, a92bdc6, 3683a78, 3f3ed75, b3a76f5, 3d5d5bb, cd97e30, d828b2e)
✅ login: Jawabiscuit / name: Jonas Avrin (8d79945, ff5c1f5, 9ee6346, f5e9b7a, a9aeb94, 9fa4af6, c13efbd, 8a2040c, a8f2f7d, 5a62599, af4365d, 9bde403, f7f06b1, ee0e0bd, ff743db, 0ec2eed, 0ef8df0, 0186176, d894bb3, c3e6ce5, 685ff34, e3df9b6, 41e3aa3, 3ddcfd7, b119326, c8463ff, d5510de, 332a19f, 75a904a, d05e752, 519c462, 89fa4ac, 5fab1ce, 5a28572, 4213333, 1c54516, cfd8846, 32cb146, a34dc14, d6d0efe, 08b77e9, 6fcf1d1, 4c5f1ec, 2b83e8b, fbc6e7f, 574190d, c16568a, 143cbad, c01d569, 5c9b251, 2599cbf, 0c8380b, 38a6211, 89b5119, d851bea, 04a8673, de40018, ce42276, 626cbc6, 38ab2e5, 595c9e8, 07c762a, 4fe7417, a36adcc, 63d1fe6, f5920fc, c2cac0e, 049070b, 868cfde, ee46d3c, 9d55dec, 544272a, 3da8f11, ac3ff37, d0958cd, 76cb8cb, c6fc1da, ae11f37, 3ff62e9)
✅ login: JeanChristopheMorinPerso / name: Jean-Christophe Morin (25665f5, 1778f57)

Jawabiscuit · 2023-05-05T15:59:43Z

I'm getting time in our next sprint starting next week to dive back in and get reoriented with the requirements of this PR, referencing #1364. I'll be working on other things as well, which is why it's a reorientation task. Also, I need to get familiarized a bit more with rez code and the details of unittesting. I'm optimistic I'll get to start on addressing the notes very soon.

JeanChristopheMorinPerso · 2023-05-05T16:19:35Z

Thanks for the update @Jawabiscuit !

Jawabiscuit · 2023-05-16T14:21:51Z

I'm running into a peculiar situation and I'm not sure the best way to resolve it for this PR. Help is appreciated. Here are my notes:

[!error]+

Traceback (most recent call last):
  File "/home/javrin/rez/2.112.0-pr-1475-test-0/local/lib/python2.7/site-packages/rez/tests/test_shell_utils.py", line 59, in test_path_conversion_unix_forced_fwdslash
    converted_path = convert_path(test_path, 'unix', force_fwdslash=True)
  File "/home/javrin/rez/2.112.0-pr-1475-test-0/local/lib/python2.7/site-packages/rezplugins/shell/_utils/windows.py", line 43, in convert_path
    path = _env_var_regex.sub(_repl, path)
AttributeError: 'NoneType' object has no attribute 'sub'

rez selftest failure on Ubuntu
- Shell: bash
- Errors are coming from test_shell_utils.py
- Doesn't reproduce if running unittest like I would a normal python package in VSCode
  - Python: Configure Tests -> unittest -> with "tests" or "src" directory and "test_*.py" test discovery
- Doesn't reproduce if I run rez selftest --shell_utils
  - This is because an upstream test is provoking the error
  - I did a binary search to figure the offender. It made sense after I found it that it was test_plugin_manager.py
- Problem is coming from test_plugin_manager.py and _reset_plugin_manager
  - _reset_plugin_manager runs on test case construction, setup, and teardown and deletes rezplugin modules from sys path
  - If I comment the del sys.modules[key] line in _reset_plugin_manager the testing errors downstream in test_shell_utils.py go away
    - https://github.com/AcademySoftwareFoundation/rez/blob/master/src/rez/tests/test_plugin_manager.py#L38
    - origin
      - PR: Extension plugins #1040
      - Commit: e1739e9#diff-f77ede425b34594d398abcac9aa68bfb837ce68e732c691d4010cb884d905bafR49
  - If I run the tests in this order ['rez.tests.test_release', 'rez.tests.test_plugin_manager', 'rez.tests.test_shell_utils'] the problem also clears itself up
    - The normal alphabetical order is ['rez.tests.test_plugin_manager', 'rez.tests.test_release', 'rez.tests.test_shell_utils']
    - I modified run_unittest in selftest.py to binary search (see below), adding that block and switching the test order should reproduce what I'm seeing
  - Manipulating _reset_plugin_manager so that sys modules do not get deleted after the last test runs, only surfaces another error in rez.tests.test_release
    - AttributeError: type object 'MemoryPackageRepository' has no attribute 'create_repository' (in shell 'sh')

[!example]+ selftest.py

    argv = [argv[0]] + [
        'rez.tests.test_e2e_shells',
        'rez.tests.test_release',
        'rez.tests.test_plugin_manager',
        'rez.tests.test_shell_utils'
    ]
    main(module=None, argv=argv, verbosity=verbosity)

@JeanChristopheMorinPerso @davidlatwe

davidlatwe · 2023-05-16T16:34:17Z

Hey @Jawabiscuit ,

Yeah that del sys.modules[key] indeed is dangerous. Can't remeber why I did such thing, sorry. 😅

Manipulating _reset_plugin_manager so that sys modules do not get deleted after the last test runs, only surfaces another error in rez.tests.test_release

Hm, I think that's because this plugin-manager testcase is loading a fake memory repository from testing data. And when the teardown function doesn't do cleanup, test_release is getting that fake instance.

We should have another PR to address this test module, each test case should be loading fake plugins which are named in a way that are safe to be deleted from sys.modules when the test is finished, e.g. package_repository/dum.

That said, why it's not falling on CI?

Edit

I think I've got the idea why I did that del sys.modules[key] in test. Need to verify but plugin manager should be able to load (re-import) requested plugin when it's not in sys.module.

And in your test_shell_utils.py, you are importing that convert_path function from rezplugins.shell._utils which is an internal helper that are not meant to be exposed.

We should keep in mind that, rezplugin.* can only works as a plugin, not also being a library. Does that make sense?

As for should we do del sys.modules[key] in test? I am not sure, need to get more familiar with the underlaying code and doing some tests. 💦

Jawabiscuit · 2023-05-16T22:47:28Z

Hey @davidlatwe, thanks for responding. Yeah, I'm anticipating there being issues when CI is run in this repo. I'm seeing it come up in my repo https://github.com/Jawabiscuit/rez/actions/runs/4981272972/jobs/8915311256. I'm new with GH actions, for instance I don't know why now it's saying awaiting maintainer approval. It's possible I'm seeing something that might not occur in the ASWF repo but I am able to reproduce it so I thought it's probably worth looking into.

I was thinking the same thing about the import for rezplugin as well. Do you have an opinion as to how rezplugins.shell._utils.windows gets brought in for testing? If not, I'll assume importlib should be used. Oh, and keep in mind I didn't write the tests for test_shell_utils. Thanks!

JeanChristopheMorinPerso · 2023-05-16T22:51:27Z

I don't know why now it's saying awaiting maintainer approval.

That's because we've configured GitHub to not run CI for first time contributors unless we manually approve it. I've been approving the workflows all this time in the background without you noticing. I'm usually pretty quick, but today was a pretty busy day.

Jawabiscuit · 2023-05-16T22:53:54Z

Oh, thanks @JeanChristopheMorinPerso , you were so fast I though it was automated! And sure enough... E AttributeError: 'NoneType' object has no attribute 'sub' :)

JeanChristopheMorinPerso · 2023-05-16T22:58:28Z

Hehe. I just approved them. FYI, we do this because someone could open PRs to do crypto mining, or just put a super long sleep and it would waste some precious build minutes and it would monopolize the agents. We have access to 100 parallel workflows at the organization level, which means someone could do crypto mining on 100 workers for multiple hours (or until we notice it).

So in general, it's better to disable automatic triggers for first time contributors. And I actually think that it's configured like this by default when you create a repo nowadays.

davidlatwe · 2023-05-17T06:51:53Z

👋🏼 @Jawabiscuit

Do you have an opinion as to how rezplugins.shell._utils.windows gets brought in for testing?

Just my 2 cents, I would either directly test with a real shell like this test_shell.py :: test_disabled_path_normalization(), or move that function into rez.utils and test it in here.

Oh, and keep in mind I didn't write the tests for test_shell_utils.

Ah sorry, didn't notice that and no mean to finger-pointing! 🤗

Jawabiscuit · 2023-05-17T13:15:50Z

JC, cool I have no quams with that. David, thanks, I will do something similar to what you are suggesting.

Tangentially, I think this little issue reveals how tests and plugin code are not as isolated as they should could be. In attempts to keep the PR on track, I think if I get the chance I'm feeling inspired to create a discussion to layout a proposal for structural improvements to get some feedback from contributers. I'll look more into if there are similar discussions and through the history of plugins and tests for shells in the meantime. I don't want to spark too much conversation around that subject here.

JeanChristopheMorinPerso

I transferred the comments from the previous PRs and added new ones.

JeanChristopheMorinPerso · 2023-05-27T21:17:31Z

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


From what I remember, we initially said we want it to be off by default. So I'd rename it enable_path_normalization and set the value to False.

JeanChristopheMorinPerso · 2023-05-27T21:18:50Z

src/rez/rezconfig.py

@@ -534,6 +534,19 @@
    "*PATH"
 ]

+# Some shells may require multiple types of pathing, so this option provides


I feel like this could be clarified and simplified. In its current form, it's kind of hard to understand what it really does.

JeanChristopheMorinPerso · 2023-05-27T21:19:05Z

src/rez/rezconfig.py

+# Some shells may require multiple types of pathing, so this option provides
+# a way to define variables on a per-shell basis to convert for shell pathing
+# instead of the pathing provided above or no modification at all.
+shell_pathed_env_vars = {


Maybe env_vars_to_convert or something like that?

JeanChristopheMorinPerso · 2023-05-27T21:19:56Z

src/rez/shells.py

@@ -433,7 +472,7 @@ def _create_ex():
                assert self.rcfile_arg
                shell_command = "%s %s" % (self.executable, self.rcfile_arg)
            else:
-                shell_command = self.executable
+                shell_command = '"{}"'.format(self.executable)


I'm curious, is the double quotes required or it could be single quotes? Single quote are safer because they are not evaluated.

JeanChristopheMorinPerso · 2023-05-27T21:20:41Z

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')


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

JeanChristopheMorinPerso · 2023-05-27T21:30:26Z

src/rezplugins/shell/sh.py

@@ -130,13 +143,17 @@ def escape_string(self, value, is_path=False):
                if not txt.startswith("'"):
                    txt = "'%s'" % txt
            else:
-                if is_path:
+                if is_shell_path:


What's the difference between a path and a shell path? I'm not sure to understand the difference just by looking at this code and without looking inside the functions that are called.

JeanChristopheMorinPerso · 2023-05-27T21:31:08Z

src/rez/rex.py

@@ -571,6 +586,16 @@ def escape_string(self, value, is_path=False):
    def _is_pathed_key(cls, key):
        return any(fnmatch(key, x) for x in config.pathed_env_vars)

+    @classmethod
+    def _is_shell_pathed_key(cls, key):
+        shell_name = cls.name() if hasattr(cls, 'name') else ''


In which case would name not be present?

JeanChristopheMorinPerso · 2023-05-27T21:31:48Z

src/rez/rex.py

@@ -590,13 +615,15 @@ def normalize_path(self, path):
        Returns:
            str: The normalized path.
        """
+        print_debug("ActionInterpreter normalize_path()")


Should these stay or they were purely added for local development purposes?

JeanChristopheMorinPerso · 2023-05-27T21:33:08Z

src/rezplugins/shell/_utils/powershell_base.py

+        if config.disable_normalization:
+            return path
+
+        # TODO: Is this necessary?


Is it? I personally don't know without taking a deeper look.

JeanChristopheMorinPerso · 2023-05-27T21:35:07Z

src/rezplugins/shell/cmd.py

+        converted_path = convert_path(path, 'windows')
+
+        if path != converted_path:
+            self._addline("REM normalized path: {!r} -> {}".format(path, converted_path))


Should you log too?

Jawabiscuit · 2023-06-20T23:13:41Z

Hey @JeanChristopheMorinPerso my goal this week is to mark this PR ready for review. I'm finishing up e2e unittests and documentation is the final task on my list. I'll give a thorough breakdown when I change the PR status. That said, we seem to be confronting EoL for python-2 in the images we're using for testing so now the CI is breaking, see also actions/setup-python#672

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

https://docs.chocolatey.org/en-us/choco/release-notes#breaking-changes Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

…ly fixes the issue in cmd shell. Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>

JeanChristopheMorinPerso · 2024-01-27T21:35:27Z

I took the liberty to merge the main brain in this PR to make sure that it's up to date. Tests are passing (even with the new refactored CI setup).

I had to do some debugging though, and noticed that the gitbash tests absolutely need enable_path_normalization to be set to True to pass. I'm not sure if we should be concerned by this or not since they were perfectly working before. See https://github.com/AcademySoftwareFoundation/rez/actions/runs/7679895882 (which contains the fix to make sure that shells are really tested).

Dennis-Lehmann · 2024-02-04T21:31:42Z

As requested by @JeanChristopheMorinPerso I'm reposting my comment from #1617 here.

I can confirm that the changes introduced in this PR fully address the mentioned issues.

But there seems to be a remaining issue utilizing the gitbash shell. (See further down below)

Conclusion

The error message now provides the path to the exe:

18:09:10 WARNING  Git-bash executable has been detected at c:\windows\system32\bash.exe, but this is probably not correct (google Windows Subsystem for Linux). Consider adjusting your searchpath, or use rez config setting plugins.shell.gitbash.executable_fullpath.

Also setting up a .rezconfig.py with the following content, gets rid of the warning:

plugins = {
    "shell": {
        "gitbash": {
            "executable_fullpath": R"C:\Program Files\Git\bin\bash.exe",
        }
    }
}

⚠️ The path to bash.exe is properly resolved, though requesting an empty environment using the gitbash shell results in the following issue:

PS C:\Users\Dennis> rez env --shell gitbash

You are now in a rez-configured environment.

bash: rezolve: command not found
>
Dennis@COMPUTER MINGW64 /c/Users/Dennis
$

Rez also is not available in that environment

[...]
Dennis@COMPUTER MINGW64 /c/Users/Dennis
$ rez -h
bash: rez: command not found
>
Dennis@COMPUTER MINGW64 /c/Users/Dennis
$

Jawabiscuit changed the title ~~Fixes #1302~~ Windows Gitbash Support Fixes #1302 Apr 21, 2023

JeanChristopheMorinPerso mentioned this pull request May 17, 2023

TSC Meeting 2023-05-18 #1492

Closed

JeanChristopheMorinPerso requested changes May 27, 2023

View reviewed changes

Jawabiscuit force-pushed the fixes_#1302 branch 13 times, most recently from 993f3e2 to 15ac508 Compare June 16, 2023 23:23

Jawabiscuit force-pushed the fixes_#1302 branch 2 times, most recently from 497d526 to 4a21b7a Compare June 22, 2023 17:20

Jawabiscuit added 13 commits September 15, 2023 09:32

Change CI detection

c2cac0e

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Per shell config overrides are handled by test decorator

049070b

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Newer versions of chocolatey & .NET 4.8 require restart

868cfde

https://docs.chocolatey.org/en-us/choco/release-notes#breaking-changes Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Env var seps weren't actually being overridden here

ee46d3c

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Consolidate and make more meaningful e2e shell tests

9d55dec

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Remove extra import

544272a

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Address sonarcube bug and code smells

3da8f11

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

This partially addresses AcademySoftwareFoundation#1321 because it on…

ac3ff37

…ly fixes the issue in cmd shell. Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Add cmake build test and path normalization test for Windows

d0958cd

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Address cmake build failures on Windows in powershell and pwsh

76cb8cb

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Fix e2e test for py ver < 3.3

c6fc1da

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Add Sphinx syntax where appropriate

ae11f37

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Enforce consistency between *pendenv methods

3ff62e9

Signed-off-by: javrin <jawabiscuit@users.noreply.github.com>

Jawabiscuit force-pushed the fixes_#1302 branch from c8ee458 to 3ff62e9 Compare September 15, 2023 14:47

This was referenced Oct 21, 2023

Fix CMake build in Windows for CMAKE_MODULE_PATH force to use "/" #1548

Closed

Roadmap to rez 3.0 #1558

Closed

JeanChristopheMorinPerso mentioned this pull request Nov 6, 2023

Better default for pathed_env_vars #1569

Closed

JeanChristopheMorinPerso added this to the 3.0.0 milestone Nov 16, 2023

JeanChristopheMorinPerso added 2 commits January 27, 2024 16:23

Merge branch 'main' into fixes_#1302

25665f5

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>

Fix test_shells after merge

1778f57

Signed-off-by: Jean-Christophe Morin <jean_christophe_morin@hotmail.com>

JeanChristopheMorinPerso force-pushed the fixes_#1302 branch from cebfc45 to 1778f57 Compare January 27, 2024 21:28

JeanChristopheMorinPerso added the run-benchmarks Add this label to a PR to run the benchmarks label Jan 27, 2024

JeanChristopheMorinPerso mentioned this pull request Jan 28, 2024

Missleading warning due to gitbash shell plugin ignoring its config settings #1617

Open

JeanChristopheMorinPerso removed this from the 3.0.0 milestone Feb 4, 2024

JeanChristopheMorinPerso modified the milestone: Next Apr 7, 2024

brycegbrazen mentioned this pull request Apr 17, 2024

TSC Meeting 2024-04-18 #1727

Open

brycegbrazen mentioned this pull request Jun 20, 2024

Fix missing path in GitBash warning message #1775

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Windows Gitbash Support Fixes #1302 #1475

Windows Gitbash Support Fixes #1302 #1475

Jawabiscuit commented Apr 21, 2023 •

edited by JeanChristopheMorinPerso

Loading

linux-foundation-easycla bot commented Apr 21, 2023 •

edited

Loading

Jawabiscuit commented May 5, 2023

JeanChristopheMorinPerso commented May 5, 2023

Jawabiscuit commented May 16, 2023

davidlatwe commented May 16, 2023 •

edited

Loading

Jawabiscuit commented May 16, 2023 •

edited

Loading

JeanChristopheMorinPerso commented May 16, 2023

Jawabiscuit commented May 16, 2023 •

edited

Loading

JeanChristopheMorinPerso commented May 16, 2023

davidlatwe commented May 17, 2023 •

edited

Loading

Jawabiscuit commented May 17, 2023

JeanChristopheMorinPerso left a comment

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

JeanChristopheMorinPerso May 27, 2023

Jawabiscuit commented Jun 20, 2023

JeanChristopheMorinPerso commented Jan 27, 2024

Dennis-Lehmann commented Feb 4, 2024

Windows Gitbash Support Fixes #1302 #1475

Are you sure you want to change the base?

Windows Gitbash Support Fixes #1302 #1475

Conversation

Jawabiscuit commented Apr 21, 2023 • edited by JeanChristopheMorinPerso Loading

Fixed

Changed

Adds

Observations

Testing

Experience

Testing

How to Test/Enable

Logging Settings

Path Normalization Settings

Resolved Context Execution

Source Code Execution

Effects of Normalization for Other Shells

Resolved Context Execution

Source Code Execution

Known Issues

Questions

Related PRs

🕶 Recommended reading

linux-foundation-easycla bot commented Apr 21, 2023 • edited Loading

Jawabiscuit commented May 5, 2023

JeanChristopheMorinPerso commented May 5, 2023

Jawabiscuit commented May 16, 2023

davidlatwe commented May 16, 2023 • edited Loading

Edit

Jawabiscuit commented May 16, 2023 • edited Loading

JeanChristopheMorinPerso commented May 16, 2023

Jawabiscuit commented May 16, 2023 • edited Loading

JeanChristopheMorinPerso commented May 16, 2023

davidlatwe commented May 17, 2023 • edited Loading

Jawabiscuit commented May 17, 2023

JeanChristopheMorinPerso left a 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

Choose a reason for hiding this comment

Jawabiscuit commented Jun 20, 2023

JeanChristopheMorinPerso commented Jan 27, 2024

Dennis-Lehmann commented Feb 4, 2024

Jawabiscuit commented Apr 21, 2023 •

edited by JeanChristopheMorinPerso

Loading

linux-foundation-easycla bot commented Apr 21, 2023 •

edited

Loading

davidlatwe commented May 16, 2023 •

edited

Loading

Jawabiscuit commented May 16, 2023 •

edited

Loading

Jawabiscuit commented May 16, 2023 •

edited

Loading

davidlatwe commented May 17, 2023 •

edited

Loading