Skip to content

Switch from AppVeyor to GitHub Actions.#2072

Merged
daviesrob merged 1 commit into
samtools:developfrom
jkbonfield:windows-actions
Jul 16, 2024
Merged

Switch from AppVeyor to GitHub Actions.#2072
daviesrob merged 1 commit into
samtools:developfrom
jkbonfield:windows-actions

Conversation

@jkbonfield

Copy link
Copy Markdown
Contributor

Also changed test/** to text mode as Actions enforces windows cr-lf on git clone, unlike AppVeyor's git. We could be more selective in this, but this replicates what we had before.

[If we wish to cross-validate our support for e.g. SAM in both line endings, then this should be done by explicit files with both types so we can validate on both platforms, but I don't know if we wish to support that anyway.]

At the time of writing (NB it's late afternoon UK time so during peak US time for github), GitHub Actions completed in ~6 mins while AppVeyor took approx 12+ minutes (only 10 of which was running with 2-3 spent waiting for a runner to be available).

@daviesrob daviesrob self-assigned this Jun 27, 2024
@jkbonfield jkbonfield mentioned this pull request Jul 4, 2024

@daviesrob daviesrob left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Again, it would be useful to leave Appveyor in place for a few days while we swap over.

Comment thread .github/workflows/windows-build.yml Outdated
Comment on lines +38 to +39
./configure
make -j32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Configure should use --enable-werror so warnings will be obvious.
make -j32 seems a bit ambitious given that the runners only have 4 CPUs.

Comment thread .github/workflows/windows-build.yml Outdated
Comment on lines +48 to +49
./configure
make -j32

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Seem comments on the "Compile htslib" section.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I switched to make -j6. I know it has 4 CPUs, but often with mingw it becomes I/O bound so over-committing on CPUs can speed things up.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'll try a -j4 and look at timings too :)

@jkbonfield jkbonfield force-pushed the windows-actions branch 2 times, most recently from 3a131cf to 07189fb Compare July 8, 2024 16:12
@daviesrob

Copy link
Copy Markdown
Member

Comparing with the old AppVeyor logs, I noticed that libcurl had gone missing, and libdeflate was never there but is now available as an MSYS2 package. I've pushed up a commit to add them both.

One remaining issue is that HTSlib's configure gets run twice. I this should be fixable by only running autoreconf -i after cloning HTSlib, and then letting SAMtools configure and make everything itself. At least, that's how the Appveyor script used to do it, and it's probably easier than trying to work out why samtools' ./configure feels the need to run the one in htslib again.

Also changed test/** to text mode as Actions enforces windows cr-lf on
git clone, unlike AppVeyor's git.  Added back a few obvious files as
text so any PRs made will revert local cr-lf back on commit.  We could
be more explicit, but I think this covers most likely candidates.

[If we wish to cross-validate our support for e.g. SAM in both line
endings, then this should be done by explicit files with both types so
we can validate on both platforms, but I don't know if we wish to
support that anyway.]

At the time of writing (NB it's late afternoon UK time so during peak
US time for github), GitHub Actions completed in ~6 mins while
AppVeyor took approx 12+ minutes (only 10 of which was running with
2-3 spent waiting for a runner to be available).

Co-authored-by: Rob Davies <rmd+git@sanger.ac.uk>
@daviesrob

Copy link
Copy Markdown
Member

Pushed version with tweaked MSYS2 package lists, and added libdeflate. Comparing logs with AppVeyor show the same results, apart from the usage tests which are skipped due to no IO::Pty module or can't open pty on GitHub actions.

@daviesrob daviesrob merged commit f1c88de into samtools:develop Jul 16, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants