Conversation
| # clang treats backslashes in the -include path as escapes | ||
| self._user_header = Path(self._user_header).absolute().as_posix() |
There was a problem hiding this comment.
It might make sense to improve the path handling for this in cmdstan itself:
https://github.com/stan-dev/cmdstan/blob/d3d5df6a22565edbe13edbd4eb40762cc8c5a4d6/make/program#L47
For other variables we use subst to swap backslashes:
https://github.com/stan-dev/cmdstan/blob/d3d5df6a22565edbe13edbd4eb40762cc8c5a4d6/make/program#L83
There was a problem hiding this comment.
Thanks for the pointers, I'll see how to clean this up a bit.
There was a problem hiding this comment.
I had a look, and yes, it would be great to fix it there. I'll submit a PR for CmdStan too, but in the meanwhile, since we will support at least a few older versions of CmdStan, I've kept the code here and updated the comment to clarify.
WardBrian
left a comment
WardBrian
left a comment
There was a problem hiding this comment.
The changes look pretty good to me at this point, a couple comments on the tests
| def test_user_header_is_posix_path() -> None: | ||
| """Backslashes in the -include path are eaten by clang.""" | ||
| opts = CompilerOptions( | ||
| user_header=os.path.join(DATAFILES_PATH, 'return_one.hpp') | ||
| ) | ||
| opts.validate() | ||
| assert '\\' not in opts.user_header | ||
| assert opts.user_header.endswith('/return_one.hpp') | ||
| assert os.path.isfile(opts.user_header) | ||
| assert str(opts.cpp_options['USER_HEADER']) == opts.user_header |
There was a problem hiding this comment.
This test focuses us to do the update, but I think it would be better to have a test that a windows path actually compiles, which would work whether us or CmdStan do the fix up
There was a problem hiding this comment.
Agreed, I have extracted a test in tests\test_model.py (test_model_compile_user_header) for this check.
| def test_parse_cmdline_args(monkeypatch: pytest.MonkeyPatch) -> None: | ||
| monkeypatch.setattr( | ||
| sys, 'argv', ['install_cxx_toolchain', '-v', '4.5', '-d', 'somewhere'] | ||
| ) | ||
| args = install_cxx_toolchain.parse_cmdline_args() | ||
| assert args['version'] == '4.5' | ||
| assert args['dir'] == 'somewhere' | ||
| assert args['silent'] is False | ||
| assert args['progress'] is False |
There was a problem hiding this comment.
This can go away; it's basically just testing argparse
| #include <iostream> | ||
| int main() { std::cout << "hello\\n"; return 0; } | ||
| """)) | ||
| subprocess.run(["g++", "-O0", "hello.cpp", "-o", "hello.exe"], check=True) |
There was a problem hiding this comment.
I think windows runners might already have g++ in their path, so I'm not confident this is really testing all that much. Maybe it could also assert on gcc --version or which gcc?
There was a problem hiding this comment.
Agreed, I already had an assert on the gcc path in the previous step of the job, and I have added a gcc --version to this step (in addition to checking the architecture against what is expected.)
|
Thanks for the (re-)review, I have made these changes and everything is passing on my fork. |
This branch has not been deployed
2 participants
Submission Checklist
Summary
I have updated the RTools installation support on Windows, so versions up to 4.5 are now supported. Critically, that now adds native support for Windows on ARM64. I have updated the rest of the build-related code to work on both AMD64 and ARM64 versions of Windows.
The one thing worth pointing out is that I ran into 4 tests failing only on Windows ARM; this is because those particular tests were running an exe and immediately trying to delete it. On Windows 11 ARM, Windows Defender cannot be disabled, per the readme; and Windows Defender tries to scan every executable when it is run, so deleting it immediately fails.
We can choose to not add Windows ARM to the main workflow, as that workflow has a subset of supported platforms anyway. In that case I can remove the changes to main.yml and the delete-retry workaround, let me know.
I also added lots of tests and a new workflow to test the toolchain installation on Windows.
Copyright and Licensing
Please list the copyright holder for the work you are submitting (this will be you or your assignee, such as a university or company):
Nikhil Dabas
By submitting this pull request, the copyright holder is agreeing to license the submitted work under the following licenses: