wavebitscientific / datetime-fortran

Date and time manipulation for modern Fortran
MIT License
137 stars 51 forks source link

BUGFIX: MinGW, platform-agnostic CI #70

Closed scivision closed 3 years ago

scivision commented 3 years ago

These changes make all tests pass on Windows MinGW/MSYS

CI move

Travis-ci.org is shutting down. Travis-CI.com now has quota even for open-source. Move CI to GitHub Actions (you need to enable this from this repo Actions tab).

Make CI be Windows, Mac, Linux to avoid platform-specific issues

lint / cleanup

milancurcic commented 3 years ago

Thank you for these improvements and especially Windows and CI fixes.

I'm only not in favor of https://github.com/wavebitscientific/datetime-fortran/pull/70/commits/fb39ee972a51dec8de1c2c0d7df91304b53f373b.

As I understand it, the only pro is:

And the cons are:

So I suggest reverting this commit. Unless you can convince me otherwise, I'm quite open to convincing :).

milancurcic commented 3 years ago

Here's another reason why I still like writing pure elemental even when pure is redundant with elemental:

I consider pure Fortran procedures to be overall beneficial for any code base. They help make cleaner code, and may even help compilers optimize.

When I write pure elemental, I'm also signaling and promoting the use of pure wherever possible. When you write just elemental, yes, they're still pure, but it's implied. The reader needs to be aware of this rule. When you write pure elemental, the word pure is right there in your face.

So it's kind of a hand-wavy, persuasion-based argument, but I thought it worth mentioning.

scivision commented 3 years ago

the "pure elemental" change is really bikeshedding or like 2 vs 4 spaces ha ha. I dropped that commit

scivision commented 3 years ago

So to make the new CI work, please go to the Actions tab at the top of this repo and enable. It might take one more commit to the *.f90 code to make the CI trigger then.

milancurcic commented 3 years ago

Great. I went to Actions but I don't see a simple "Enable" button or something similar. Wasn't sure what to do so I added you as a repo maintainer. Do you mind doing it? :)

I tested this PR with GNU and Intel compilers on Linux and all looks good on my end. I trust you with the Windows stuff.

scivision commented 3 years ago

OK thanks. They've changed the interface, I will figure it out. I was an early Actions user and I enabled it once and i'll see what it needs now.

scivision commented 3 years ago

OK I had to make a dummy PR enabling Actions which I then closed. Now Actions is enabled.

milancurcic commented 3 years ago

Please merge when you're satisfied with this. I am. Thanks again!

scivision commented 3 years ago

the "needs" parameters in the CI.yml are just to be a good citizen--they avoid wasting MacOS and Windows workers, by waiting to be sure the "cheaper" Linux CI worker passes first. I do this even if it's all Linux workers, avoid needlessly failing a whole bunch of CI by doing sanity checks first.

milancurcic commented 3 years ago

I'm confused how did my commit from yesterday not make it into this PR. I pushed it yesterday:

commit 2d02318728bbba5de51450f5d73bd8cb311a7a9d (HEAD -> redun, origin/redun)
Author: milancurcic <caomaco@gmail.com>
Date:   Thu Dec 17 10:05:07 2020 -0500

    bump version to 1.7.1

@scivision do you know?

scivision commented 3 years ago

I rebased and our commits may have overlapped in time. I.e. I erased the commit unknowingly

scivision commented 3 years ago

So I think just recreate the commit and push again? Hopefully it was just a couple lines..

milancurcic commented 3 years ago

Yes, it's an easy and small commit. I will send it soon.

I'm more shocked that git allowed this. My commit has disappeared without a trace. But then my git knowledge is limited. Maybe this is what rewriting commit history does.