search

samtools / htslib

C library for high-throughput sequencing data formats
Other
783 stars 447 forks source link

Windows builds via GitHub Actions #1796

Open jkbonfield opened 5 days ago

jkbonfield commented 5 days ago

The AppVeyor builds have become slower to launch and they're also a bit slow on execution. GitHub Actions runs in about half the time.

Note this needs https://github.com/samtools/htscodecs/pull/123 merging first, but for purposes of testing I just incorporated it in this PR via an extra commit to change the submodule commit. If the htscodecs PR gets modified in any way, changing the hash, then I'll update this appropriately.

Also, if you wish to see these tests in-situ here, we could copy this PR branch to samtools/htslib from jkbonfield/htslib as otherwise it won't enable the workflow. (As I did for htscodecs)

jmarshall commented 4 days ago

I was initially going to suggest a git config core.autocrlf-core.eol-etc step before the checkout step (on the basis that it would be better to affect only the action runner rather than all Windows users as adding to .gitattributes does), but after reading through actions/checkout#135 I have changed my mind. Fixing this for the test files for all Windows users is actually the right thing to do!

That's a long thread, but https://github.com/actions/checkout/issues/135#issuecomment-613342603 makes a very good point that just doing a blanket ** -text that would also affect *.c files would be the wrong thing to do. So I'm glad to see this PR just does ‑text more specifically for a bunch of test file extensions.

jkbonfield commented 4 days ago

I confess for the Samtools one I gave up worrying about every little extension and just went for test/**, but I do accept there are some C files in there. I wonder if we can then do text/*.c +text or similar to reenable it as an exception to the blanket rule? I doubt it matters though tbh as I'm not convinced we'll be getting many, if any, PRs from windows developers, and even if so the code tools typically all support nl anyway.