Allow building with cmake on Windows - #2090
Conversation
a548e19 to
ce9ff60
Compare
maxnbk
left a comment
There was a problem hiding this comment.
Nicely done contribution.
Apart from this PR, we could use actual tests that use/generate the hello_world package, but I won't consider that a blocker for this particular PR merge.
I made a few suggestions are mostly cosmetic, or affect the way docs generate.
The only open question is the CMAKE_PREFIX_PATH one, or CMAKE_*_PATH, alternatively. I suspect the right thing to do would be the wildcard, since this is a new config value, and if someone needs to override the default config value, the result is effectively the same. I'm not just sure if there are any CMAKE path vars that are supposed to have the same treatment.
| # This setting identifies environment variables that should NOT have path | ||
| # normalization applied, even if they match a pattern in ``pathed_env_vars``. | ||
| # This is useful for variables like ``CMAKE_MODULE_PATH`` which end in ``PATH`` | ||
| # but require forward slashes regardless of the shell. Wildcards are supported. |
There was a problem hiding this comment.
| # but require forward slashes regardless of the shell. Wildcards are supported. | |
| # but require forward slashes regardless of the shell. Wildcards are supported. | |
| # Takes priority over :data:`pathed_env_vars`. |
| @@ -558,6 +558,8 @@ def escape_string(self, value, is_path=False): | |||
|
|
|||
| @classmethod | |||
| def _is_pathed_key(cls, key): | |||
There was a problem hiding this comment.
| def _is_pathed_key(cls, key): | |
| def _is_pathed_key(cls, key): | |
| """Return True if ``key`` is a path-like env var subject to normalization.""" |
| ] | ||
|
|
||
| # This setting identifies environment variables that should NOT have path | ||
| # normalization applied, even if they match a pattern in ``pathed_env_vars``. |
There was a problem hiding this comment.
| # normalization applied, even if they match a pattern in ``pathed_env_vars``. | |
| # normalization applied, even if they match a pattern in :data:`pathed_env_vars`. |
| # This is useful for variables like ``CMAKE_MODULE_PATH`` which end in ``PATH`` | ||
| # but require forward slashes regardless of the shell. Wildcards are supported. | ||
| non_pathed_env_vars = [ | ||
| "CMAKE_MODULE_PATH" |
There was a problem hiding this comment.
Should we consider "CMAKE_*_PATH" or an additional "CMAKE_PREFIX_PATH"? It feels to me like it makes sense to do so.
There was a problem hiding this comment.
In my testing, I've only needed to set the one variable, but I'm happy to change it to your suggestion for flexibility!
| "*PATH" | ||
| ] | ||
|
|
||
| # This setting identifies environment variables that should NOT have path |
There was a problem hiding this comment.
| # This setting identifies environment variables that should NOT have path | |
| # This setting identifies environment variables that should not have path |
| ``CMAKE_MODULE_PATH`` requires forward slashes regardless of shell, and backslash conversion will | ||
| break CMake. You can exclude variables from normalization using the :data:`non_pathed_env_vars` | ||
| config setting, which takes priority over :data:`pathed_env_vars`. By default, | ||
| ``CMAKE_MODULE_PATH`` is already excluded. Both settings support ``fnmatch``-style wildcards. |
There was a problem hiding this comment.
| ``CMAKE_MODULE_PATH`` is already excluded. Both settings support ``fnmatch``-style wildcards. | |
| ``CMAKE_MODULE_PATH`` is already excluded. Both settings support :func:`fnmatch.fnmatch`-style wildcards. |
b08de0c to
e73f789
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2090 +/- ##
=======================================
Coverage 61.29% 61.29%
=======================================
Files 164 164
Lines 20568 20571 +3
Branches 3575 3576 +1
=======================================
+ Hits 12607 12609 +2
- Misses 7089 7090 +1
Partials 872 872 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
- Introduced `non_pathed_env_vars` configuration to exclude specific variables from path normalization - Updated `_is_pathed_key` method to respect the new configuration - Added tests to verify behavior of non-pathed environment variables in Windows shell Signed-off-by: Gabriel Reed <gabrieljreed@gmail.com>
Signed-off-by: Gabriel Reed <gabrieljreed@gmail.com>
Signed-off-by: Gabriel Reed <gabrieljreed@gmail.com>
Signed-off-by: Gabriel Reed <gabrieljreed@gmail.com>
- Documentation improvements - Replace default value of non_pathed_env_vars from `CMAKE_MODULE_PATH` to `CMAKE_*_PATH` Signed-off-by: Gabriel Reed <gabrieljreed@gmail.com>
e73f789 to
585c224
Compare
Resolves #1321
Resolves #2123
Premise
Path normalization
On Windows, rez applies path normalization (converting forward slashes to back slashes) to any environment variable whose name matches the pattern in the
pathed_env_varsconfig setting, which defaults to*PATH. This is correct behavior for variables likePATH,PYTHONPATH, andLD_LIBRARY_PATH, which need native Windows path separators. However,CMAKE_MODULE_PATHalso matches*PATH, but Cmake requires forward slashes in this variable.Unterminated string literal in
rez_install_pythonWhen building rez packages on Windows that use
rez_install_python(withnmake), the build fails with a PythonSyntaxError: unterminated string literalThe root cause is in
InstallPython.cmake- theadd_custom_commandthat compiles.pyfiles into.pycuses single quotes around the Python-cargument.On Unix, make dispatches commands via
/bin/shwhere single quotes are valid string delimiters. On Windows,nmakedispatches viacmd.exe, where single quotes are literal characters, so Python receives'importas the start of an unterminated string literal.Changes
non_pathed_env_vars, which is an exclusion list that takes priority overpathed_env_vars.pathed_env_varsfnmatchwildcards aspathed_env_varsCMAKE_MODULE_PATHout of the box, since this is a known case where we don't want normalizationinstall_pythonmacro-cargumentVERBATIMkeyword toadd_custom_commandso Cmake handles all platform-specific argument quoting correctlyTests
hello_worldexample package on both Windows and Linux after settingbuild_system = "cmake"ActionInterpreter._is_pathed_keyandshell.set_envrez_install_pythoncmake macro, but I verified that it worked manually