rez Windows Gitbash Support Fixes #1302

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

Fixed

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

Changed

[x] Path normalization
- [x] Off by default
- [x] /A/b/c rex paths are normalized to /a/b/c in Gitbash
- [x] Prevent paths with mixed slashes and backslashes in all shells
- [x] Do not automatically normalize {root} or this.root
- [x] {root} or this.root provides the platform normalized canonical path
- [x] Revert #1364 filepath normalization changes in sourcecode.py which were too broad and not fixing any issues
[x] Configuration
- [x] Make platform pathed and shell pathed vars distinct from one another
- [x] co-opt warn_shell_startup to enable shell setting validation and logging warnings or errors
- [x] global enable_path_normalization to turn off all normalization

Adds

[x] bash-style #!/usr/bin/env bash style shebangs which are supported in gitbash, and gitbash can execute files with/without file ext much like Unix
[x] cygpath emulation
- [x] Refactored rezplugins/shell/_utils/windows.py
- [x] cygpath.py and uncpath.py created to emulate cygpath
- [x] cygpath.py used specifically for gitbash path normalization
- [x] Better UNC and long path handling for gitbash
[x] Configuration
- [x] shell_pathed_env_vars, a per-shell pathed_env_vars setting
- [x] debug_shells to enable debug logging for all shells, includes path normalization messages
- [x] debug_cygpath to enable debug logging for the cygpath module
[x] Rez bind
- [x] --py-script-mode and argument to bind added, see testing observations
[x] Unittests
- [x] Similar to setting exclude on the per_available_shell decorator and to ease testing for a minimal number of available shells, enable use of a small list of included shells per test
- [x] Due to path normalization being disabled by default, and to keep shell tests DRY for gitbash, overriding the configuration on gitbash shell tests was added to the per_available_shell decorator with comment to remove if & when normalization is enabled by default
- [x] assertRaisesRegexp is used and mapped to assertRaisesRegex for backwards compatibility with Python 2
- [x] Many normalization / cygpath emulation tests
- [x] Shell tests
- [x] Shell e2e tests
- [x] Fixes some existing Windows unittests as well as most shell tests that were reporting false positives. It appeared to have been assumed that, in unittests when using the per_available_shell decorator, execute_shell would execute the intended shell when it was actually picking up the default system shell every time.
[ ] as_path and as_shell_path rex functions with as_shell_path(pathish, shell=bool). No time to implement, but works fine without this feature. I wouldn't mind taking a crack at it in a future PR b/c I do see its benefits.

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

Apr 21 '23 22:04 Jawabiscuit

The committers listed above are authorized under a signed CLA.

:white_check_mark: login: amorphousWaste (9f0d5ee4e20855d055eb2b49888694a1a6a5fb73, bfc99e238471c0d1e391173d3f8102920e59d13a, 8eb7f5ab7eea1662af85729e2e8aa05351ce622a, 7d66f001dfb4382df81603cfdce9946aba8ad364, 1e1d4b2d05c5452154def831b1247e798e052bca, ff7a11c8e50b67da2ac8b7bdca53f76d4741ff18, 0ce60ab949290eb8ea1edd2fd35e6be48299505d, a91a4c58d501e5a492134eb500d525f1036ab9a1, f3e2b688bdf8b3c0a5fe07737023143be8fba33e, 744e8f9f9df3e1ba45e0a09d1c4f3463ee28b40e, 7aaaecba9f73ad29b19952f8bd0ff9988c8f8308, 640e44c0730e6ea06d84a45d0302d9eac7653321, c55ee1b903dd66dc315cd627a56cdd228227f20c, a9d4de5b3f5ab763b7ff7210fcc370f9cab58124, 70576324819058595797aa88d832b3f1ba99080b, 2a51e0c668ea2b78a3bcf63322f837ca312ba582, 7f20c9ebc8858e839652655b541e1902fbc9382f, e37e7f27e5e460eca7552046b4f80feebf6ab57f, bb3bb0d4813a05dfb7bd7f0b8df9401e7acf0b82, 51d9af007a420a01ab816dfabac62339976c226c, e997c9e20b538461a7cf007513015fe4b8ca2eb3, f9607b5d480187d52c170f6a3ac8ad9ad7487a86, 6834fb269475c41192e57e4b69384f53855b30aa, 1e5b3f21f5e9e68ef4000d3b3e8b288a3810df25, 4976105e67a0cb78b9058283c35ab8890a7509c3, e12ef776a4318b884c427c99bc454aa62ef15867, af69b8de5d127d332e01acb51f92a97ea12dcfb4, 489b7bcdca9a30005f2e6b7159e4bbb7ba2acc53, 0b5594c34170423ffe68f4fd3a68b06845b87ac1, c7e74545befb0d7d355d08f05abe460ad7cb7983, 42e37f9d2d9aebf0845f0055fb268483b2e1fa4b, 267c3e333f2b9e62d349dc840405c746f04a19ce, 9d0bddeabc4951502268d424146d95c7bc84696e, db3010893c5eb72c240af7710cbe59f80e883945, 688337a7875016c02fa5352a323e942019d76ce1, 3b6b3c52820a0de3ed434add0668d3c41cecd624, a92bdc6946aebf685104660d4edf8370632ce940, 3683a78d76cd4c654200cf5d7ecf7f307cf4c15a, 3f3ed75c2919b14e86cad2f3ec88a0e9c8700c2f, b3a76f51b36671f83937266595ab0bccb3d7bf2a, 3d5d5bba6c5ec5786cbf99e9a6bd0ef3c73ade9b, cd97e306e739b9199d6053b1c00ca1f7c6c1361d, d828b2e2925658c48159ffb236199c2a4236f381)
:white_check_mark: login: Jawabiscuit / name: Jonas Avrin (8d79945e655443ed1481beecd85e521614acdf1a, ff5c1f543b94ada7b6ef156248a06dc0250b2bda, 9ee6346c122b74f66ba840efa710c24d244beaea, f5e9b7aa02aad1b94cd1f2cfe239aed8978575e9, a9aeb94fd5852c00b843ba86794a7a6cd475bce9, 9fa4af6b0a72f0cef4d0b02655513f6b2a54dbbb, c13efbd003dfb50a406e2e99f1acf7bf627ded5b, 8a2040ce90ca7980866d58a5844c01188f2996f7, a8f2f7d5fb23c14f69d0f0dd6d007d69c71c6de2, 5a62599f76810f4e969b39df92a468e911c5f855, af4365d66eb61e7cf5d372bab3fdd90dd0362dbc, 9bde403f248340aca901b9dfb4903e562f8ea1a0, f7f06b1ef04e335a8474413de13fb96ad3cbf07e, ee0e0bd12cc0f6bc83697b2d1f534ed81f061a38, ff743dbb6671cfae619976953993a3b954bb68c4, 0ec2eede1482371c3012243f01b0eb33bf11c22a, 0ef8df0808f359a04eec3e3bdd4cfc0ec36b1321, 018617665050d54ed604715791f4a410700cd5a1, d894bb3ae8d3f0f61da8c9beea08f46caf3ac5ae, c3e6ce52b7ae864ded60273175e5aa8895f6d18a, 685ff3417ff4bd517fc9fb10cb191fd837cd0fd6, e3df9b60d98129158874ac147a27fc1bba30b2e6, 41e3aa39c9703fba0d32221afa9fd25ce533d379, 3ddcfd7ff84001e9ebccb4b3b6ae7e657522704a, b1193261d7a83e2165d7c95f9df2e927f07d5f58, c8463ff1fdb916ab3fed380aa112192eef509328, d5510de2b4060d24f2466eb739f825ca9e25b1bd, 332a19f7a58f2ee7fefc743eaa671c0c1357cec3, 75a904a0ef69e93d4296a557df02d38fbbe80ee2, d05e7527ad1803546931381b0d207ef150e4cd7f, 519c46275c742204d33bdc070fb2b768b78a71d8, 89fa4ac10c52ad26e17f9a3660778b7e9c0a5f12, 5fab1ce22732810235944513db48f2754b7c62a2, 5a28572b04bf155a80e46fe99b5d63b89db36081, 42133338b6bbcc82fcbb9bc18e59d401873ee8d5, 1c54516aaa0a4e3cae5556bd0d962c8cb78ebb4a, cfd88466b281d4aecd93b7d2ad37e30f07b2a425, 32cb14698b32f274419de8bc58d634eb7039fa49, a34dc14119a9a46cd8c74f2226d7355069432e57, d6d0efeea1666f0e4c71a239ee53ef5605c655c3, 08b77e93f7ae213f7d93db7af1b45c1a06fc1bcc, 6fcf1d11fc9dce65d0b4018f04ccfc486d9a33cf, 4c5f1ec7586a673125dd5cfcfcd38b585f5ad420, 2b83e8b906444fd463b809c22102c085b10ee543, fbc6e7fa73a990856aa2aa1b064762ae1ca469ff, 574190db504b667a1e829585772ba93b7773a1f3, c16568a8d4372c2b8b6779eae3eff983b8ed1ea9, 143cbadaa9b0477b86c6f004a290124ecff7793d, c01d569a72300fa9aa163b40fdf0da7b31296e48, 5c9b251867cf4e7ed2e33071dedce79af110fa4f, 2599cbff73f9b8011bb1156bd655640110ed690b, 0c8380b58b5f2e4ce250debdd82cd4d2d49f9f53, 38a6211de97cce333a065c99a41fc1f926fc1820, 89b511942affd03dc4535bba7304692cb4d089f4, d851bea32fbea0219aaec840f872a31fe6867efe, 04a86733b64a2b9028086037d53ca3854d55fdde, de400187cea4293655f4b9e14726327ba1d9aeb1, ce42276913e1b0a336d3a69add6d6fbcb6f1d081, 626cbc63d3ac4cabb8862014ab34c0a10f32b15d, 38ab2e5cca1ddfd04d12d587cba76d5f1bafa418, 595c9e84d1e37b8302d678f92c01c2a6f3978c5e, 07c762ada7364e5c468b91efb1884ed865472985, 4fe7417c2dd74df372198c01aed9962e32aa05ad, a36adcca70702958e388d7a060103e323cb8bf9b, 63d1fe6b21ac3ae3baf9c10f1b862df412d6cb1f, f5920fc56609df996d1533d60964eee7e363c851, c2cac0e12e9a2dd5c8c267a333371eb6f8627be8, 049070bb68ca2f3d9686d18c407c8d1d46325e5e, 868cfde5b528da2bbdb0863037f0ff94d7a92356, ee46d3c82dd38dd982ec99496e3303ec7890b5da, 9d55decccd329311d29a94e9f954d62543af7cfd, 544272a40ecea967d3c8edfc1ca9f9f014effd92, 3da8f115da6a2fdb7f306549218c3e909b3558c1, ac3ff37d1951896a4174e84be16bb7923b498e4e, d0958cda36b51ee12f79a7f9f3220a784286866b, 76cb8cbc7e3b7d6f9169ac9cd4ea41d62a097043, c6fc1daaa2b6e0ebf4c9b706df90bc332440ad4d, ae11f37dfc0e744d54252abf90e89b39812374e4, 3ff62e95e73fba00abf5a26effed3cb747a92f00)
:white_check_mark: login: JeanChristopheMorinPerso / name: Jean-Christophe Morin (25665f5a4ca7c421ddad26889d3540877c84c93e, 1778f578529c606ce4f8ac14815ff919fb61fd6b)

Apr 21 '23 22:04 linux-foundation-easycla[bot]

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.

May 05 '23 15:05 Jawabiscuit

Thanks for the update @Jawabiscuit !

May 05 '23 16:05 JeanChristopheMorinPerso

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: https://github.com/AcademySoftwareFoundation/rez/pull/1040
      - Commit: https://github.com/AcademySoftwareFoundation/rez/commit/e1739e93865263924e170c81bfb60a9260c1a62c#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

May 16 '23 14:05 Jawabiscuit

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. 💦

May 16 '23 16:05 davidlatwe

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!

May 16 '23 22:05 Jawabiscuit

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.

May 16 '23 22:05 JeanChristopheMorinPerso

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

May 16 '23 22:05 Jawabiscuit

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.

May 16 '23 22:05 JeanChristopheMorinPerso

👋🏼 @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! :hugs:

May 17 '23 06:05 davidlatwe

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.

May 17 '23 13:05 Jawabiscuit

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 https://github.com/actions/setup-python/issues/672

Jun 20 '23 23:06 Jawabiscuit

Just a FYI for anyone reading, I might be unresponsive next week as I'll be on vacation.

Jun 26 '23 21:06 Jawabiscuit

SonarCloud Quality Gate failed.

2 Bugs
0 Vulnerabilities
0 Security Hotspots
9 Code Smells

No Coverage information
0.0% Duplication

idea Catch issues before they fail your Quality Gate with our IDE extension SonarLint

Jul 18 '23 21:07 sonarqubecloud[bot]

Hi @Jawabiscuit, can you rebase your branch with the master branch please? It will fix the Python 2.7 tests in CI.

Aug 27 '23 17:08 JeanChristopheMorinPerso

Great, all checks are green!

Aug 28 '23 01:08 JeanChristopheMorinPerso

Great, all checks are green!

Yes, outstanding work 👍

Aug 28 '23 18:08 Jawabiscuit

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

Jan 27 '24 21:01 JeanChristopheMorinPerso

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
$

Feb 04 '24 21:02 Dennis-Lehmann