ci: enable testing on Windows with GCC #291
Conversation
|
@gnikit Thanks so much this is wonderful. Do you mind to set the target of the PR to the new branch feature-add-windows-CI that I just made? |
|
@skosukhin Alerting you to this which will help tons in building conda feedstocks for the libraries |
Yes, no problem. I also just noticed that a few debug commits got pushed in this remote. I will drop them. As for the conda-forge feedstock, let's make an Issue to track progression and have any necessary discussions. |
|
@RobertPincus there are a series of conflicts with the new base branch, is it behind main by any chance? |
|
That's all right. I can rebase/recreate to |
- Compilers are installed via the GA fortran-setup (choco in the backend) - Version of NetCDF via APT has been replaced with conda, where possible and packman on Windows - Consequently, include and link directories need to be setup at runtime for variable expansion to occur - In the Makefiles changes have been made to properly handle variable expansions and bash-like regular expressions
|
@skosukhin This PR puts Windows and Linux CI in the same file - I'm wondering if that's wisest, or if we should create Windows-specific files? |
The majority of the code between the Linux and Windows GA workflows is the same, so you would essentially double your maintenance cost were you to split them in two files. |
|
Yes, I saw that @gnikit but it also seems like you have to do some shenanigans to adapt the Windows workflow to Linux (e.g. renaming files) and vice-versa (e.g. using chocoltey? to manage compilers). We find that people use the Linux workflow as a guide to set up local installations so I wonder if something that was more Windows-like would be a better example in the long run? (The big caveat is that Gnu make seems to be very not-like-Windows, but maybe we'll move to CMake or similar over time). |
|
No strong opinions on my side by @skosukhin has lots of experience with the CI... |
There was a problem hiding this comment.
At first, having the tests for Linux and Windows in the same file looked like a good idea to me but the difference in the steps that are run on Linux and Windows is quite big to consider moving the Windows tests to a separate file. The CI scripts are often the best and the most consistent sources of information on how to build and run things. I am afraid, that we might lose that. I don't insist though.
I am also not sure if we need to test on Windows at all. Do we really?
| - nodefaults | ||
| dependencies: | ||
| - python=3.11 | ||
| - numpy |
There was a problem hiding this comment.
| - numpy |
Why is the name environment-ci.yml better than environment-noplots.yml? What is the benefit of the channels entry? Is it relevant to CI only or can we simply add it to the existing environment files?
There was a problem hiding this comment.
Why is the name
environment-ci.ymlbetter thanenvironment-noplots.yml
Either names are fine. I didn't want to delete your conda config files, which is why I made another environment.
What is the benefit of the
channelsentry?
It is a good idea to instruct conda from which channels to fetch your packages, e.g. netcdf is not available in default it's only available in conda-forge. This is true for pretty much all your packages. Channels help make your env reproducible, so IMO you should be specifying it everywhere. Docs link to channels.
Unrelated to the previous bit, numpy has been added because it is needed in compare-to-reference.py.
| @@ -57,10 +57,10 @@ tests: rrtmgp_allsky | |||
| $(RUN_CMD) bash all_tests.sh | |||
|
|
|||
| check: | |||
| $${PYTHON-python} ${RRTMGP_ROOT}/examples/compare-to-reference.py --ref_dir ${RRTMGP_DATA}/examples/all-sky/reference --tst_dir ${RRTMGP_ROOT}/examples/all-sky \ | |||
| $${PYTHON-python} "$(RRTMGP_ROOT)/examples/compare-to-reference.py" --ref_dir "$(RRTMGP_DATA)/examples/all-sky/reference" --tst_dir "$(RRTMGP_ROOT)/examples/all-sky" \ | |||
There was a problem hiding this comment.
As far as I remember, ${var} and $(var) mean the same thing for GNU Make. Are these changes decorative or does mingw-w64-x86_64-make choke with the current syntax?
There was a problem hiding this comment.
$var and ${var} are equivalent with the latter being more robust in defining variable boundaries e.g. echo ${var}foo.
$(var) is used for command execution in your shell. In this case, the usage of $(RRTMGP_ROOT) is incorrect and leads to an erroneous expansion of the variable under msys2 + mingw-make.
There are some action runs in my fork where that is demonstrated, (the link to the Action run currently escapes me).
| echo FCINCLUDE=-I ${CONDA_PREFIX}/include >> $GITHUB_ENV | ||
| echo LDFLAGS=-L ${CONDA_PREFIX}/lib >> $GITHUB_ENV | ||
| echo LD_LIBRARY_PATH=${CONDA_PREFIX}/lib:${LD_LIBRARY_PATH} >> $GITHUB_ENV |
There was a problem hiding this comment.
| echo FCINCLUDE=-I ${CONDA_PREFIX}/include >> $GITHUB_ENV | |
| echo LDFLAGS=-L ${CONDA_PREFIX}/lib >> $GITHUB_ENV | |
| echo LD_LIBRARY_PATH=${CONDA_PREFIX}/lib:${LD_LIBRARY_PATH} >> $GITHUB_ENV | |
| echo "FCINCLUDE=-I${CONDA_PREFIX}/include" >> $GITHUB_ENV | |
| echo "LDFLAGS=-L${CONDA_PREFIX}/lib" >> $GITHUB_ENV | |
| echo "LD_LIBRARY_PATH=${CONDA_PREFIX}/lib:${LD_LIBRARY_PATH}" >> $GITHUB_ENV |
There was a problem hiding this comment.
These are equivalent I believe, just different styling, so happy to change those.
| echo FCINCLUDE="-I/mingw64/include" >> $GITHUB_ENV | ||
| echo LDFLAGS="-L/mingw64/lib" >> $GITHUB_ENV | ||
| echo LD_LIBRARY_PATH="/mingw64/lib:/mingw64/bin:${LD_LIBRARY_PATH}" >> $GITHUB_ENV |
There was a problem hiding this comment.
| echo FCINCLUDE="-I/mingw64/include" >> $GITHUB_ENV | |
| echo LDFLAGS="-L/mingw64/lib" >> $GITHUB_ENV | |
| echo LD_LIBRARY_PATH="/mingw64/lib:/mingw64/bin:${LD_LIBRARY_PATH}" >> $GITHUB_ENV | |
| echo "FCINCLUDE=-I/mingw64/include" >> $GITHUB_ENV | |
| echo "LDFLAGS=-L/mingw64/lib" >> $GITHUB_ENV | |
| echo "LD_LIBRARY_PATH=/mingw64/lib:/mingw64/bin:${LD_LIBRARY_PATH}" >> $GITHUB_ENV |
Is the LD_LIBRARY_PATH variable needed here? If so, do we need the /mingw64/bin entry?
There was a problem hiding this comment.
Same comment as before for the styling, happy to change it to match.
Is the
LD_LIBRARY_PATHvariable needed here?
Yes, MSYS is not in the runner's PATH, see docs and we are not setting the -Wl,-rpath,/bath/to/libs. We have to set explicitly LD_LIBRARY_PATH.
If so, do we need the
/mingw64/binentry?
This is where the compiler's runtime libraries are. I have no idea what compelled the MSYS packagers to place them there, but I am sure there is a reason which I don't know about.
see output from my fork's Action
https://github.com/gnikit/rte-rrtmgp/actions/runs/9326569389/job/25675276582#step:4:312
/mingw64/bin/libgfortran-5.dll
| if [[ $(uname) == *"NT"* ]]; then | ||
| # Rename files ending with .exe to the same name without the extension | ||
| for file in *.exe; do | ||
| mv "$file" "${file%.exe}" |
There was a problem hiding this comment.
I think that we should not move the files but set the EXEEXT variable to .exe when on Windows and honour it in the makefiles and scripts:
%$(EXEEXT): %.o
$(FC) $(FCFLAGS) -o $@ $^ $(LDFLAGS) $(LIBS)./rrtmgp_allsky${EXEEXT} ...and so on.
There was a problem hiding this comment.
yeap, I like this more and it was also my first implementation, but then I realised I didn't know if something else in your pipelines was consuming these binaries, so I played it safe. Happy to make that change.
| # | ||
| - name: Install NetCDF-Fortran via Conda | ||
| if: runner.os == 'Linux' | ||
| run: conda install -y -c conda-forge netcdf-fortran |
There was a problem hiding this comment.
What is the advantage of installing netcdf-fortran with Conda (given that it seems to be non-portable and we do that on Linux only)?
There was a problem hiding this comment.
It's actually quite portable, see their conda-forge feedstock. All their installers across OSs and architectures are at v4.6.1. Since conda and conda-forge are great at managing such dependencies I would be inclined to use them as much as possible in this case.
The problem with Windows is that the gfortran version is rather old. You don't want to be testing with an old compiler but for packaging purposes it's fine.
Bigger picture: When you are packaging your feedstock one of the steps is to make sure that all your dependencies i.e. netcdf-fortran are available in conda-forge. There are ways around this (e.g. building from source), but the APT package manager is not one of them.
|
@skosukhin The idea of Windows testing is in part to prepare for a Conda feedstock for the libraries. @gnikit was responding to a request I made for help on a Discourse since I have no experience with and no access to Windows development environments. @gnikit had asked about moving to CMake (as you had in the past); it's starting to seem like that's a really good idea and worth the hassle of having to learn. |
|
I will try and answer your comments @skosukhin in the coming days but my availability is very limited so I would suggest you push directly onto this branch. To setup the win CI. |
RobertPincus
merged commit b735833
into
earth-system-radiation:feature-add-windows-ci
|
@gnikit Thanks for your suggestions. I've merged your changes into our branch where we might continue to make some small edits. |
Completely up to you guys. IMO this way is simpler/easier to maintain, since there is a less duplication, but this is very subjective. The right answer here is whatever is easier for you to maintain.
Personally, I prefer a well written section in the README or the docs page, since CI workflows can get rather messy, but again completely up to you. As a side note, I found building the lib extremely easy which was great since it made the task a lot easier.
For me it makes sense to test it if you expect to have/already have users on Windows. It builds up a sense of security and trust to your users. |
|
@RobertPincus @skosukhin I think I finished with my replies (also saw that this got merged in). Always happy to chat more on this, so feel free to ping me if you need something (I might be slow to reply but I'll do my best). |
and packman on Windows
for variable expansion to occur
and bash-like regular expressions
Additional Notes
environment-noplots.ymlis not used anymore so if you have no use for it I could delete it.use-only-tar-bz2istrue.-fcheck=all -finit-real=snan -finit-integer=-999999 -finit-character=0 -ffpe-trap=invalid,zero,overflow -fbacktrace. The-ffpe-trap=...options cause for a segfault which I am guessing are indicative of a bug.