Closed gnikit closed 2 months ago
@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
@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?
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?
@gnikit Seems like you branched from main
before commit 41c5fcd? With that commit develop
and main
should have been synced, though there are a few mall changes at first.
That's all right. I can rebase/recreate to earth-system-radiation:feature-add-windows-ci
just to be safe
@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?
@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...
@skosukhin ?
@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.
@gnikit Thanks for your suggestions. I've merged your changes into our branch where we might continue to make some small edits.
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.
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.
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.
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.
I am also not sure if we need to test on Windows at all. Do we really?
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).
Additional Notes
environment-noplots.yml
is not used anymore so if you have no use for it I could delete it.use-only-tar-bz2
istrue
.-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.