Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Windows builds via GitHub Actions #1796

Merged
merged 2 commits into from
Jul 4, 2024

Conversation

jkbonfield
Copy link
Contributor

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 samtools/htscodecs#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
Copy link
Member

jmarshall commented Jun 26, 2024

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 actions/checkout#135 (comment) 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
Copy link
Contributor Author

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.

@daviesrob daviesrob self-assigned this Jun 27, 2024
@jkbonfield jkbonfield marked this pull request as ready for review June 27, 2024 13:34
run: |
export PATH=/mingw64/bin:$PATH
export MSYSTEM=MINGW64
make check
Copy link
Member

Choose a reason for hiding this comment

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

Compared with the old script, we've lost make test-shlib-exports. Assuming it does actually work on Windows, it's probably useful to have.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good spot. I'm assuming it does work as it was in the appveyor config. I'll put it back and see what happens.

@jkbonfield
Copy link
Contributor Author

Hmm, disappointingly adding back the test-shlib-exports now fails:

Checking shared library exports
kh_begin
kh_clear_##name
kh_del_##name
kh_destroy_##name
kh_get_##name
kh_init_##name
kh_put_##name
kh_resize_##name
ks_destroy
ks_getuntil2
ks_init
kseq_destroy
kseq_init
kseq_read
Error: Found unexported symbols (listed above)

I initially thought they're all erroneous things from the templates-in-C style of klib, but kseq_read is more normal, although it's still done as a macro with a SCOPE define. I'll need to dig deeper into what it's attempting to do and why we're no longer passing this script.

@jkbonfield jkbonfield force-pushed the windows-actions branch 2 times, most recently from b5f0997 to 8ca61a7 Compare July 4, 2024 11:09
Also amend .gitattributes file for more Windows text-mode removal as
for some reason the GitHub Actions git is much more likely to use
CR-LF line endings.

Added libdeflate to the windows build.

Corrected test/header_syms.pl to work on windows line endings.
@jkbonfield
Copy link
Contributor Author

Fixing the header checking perl script to cope with windows line endings worked. So I've patched that and done some rebasing and squashing of commits. Assuming it now passes this is ready for re-review.

@daviesrob
Copy link
Member

As foretold, the htscodecs submodule commit needs to be adjusted. It also might be good to leave the AppVeyor configuration in place for now, for a less messy transition. Once the couple of recent pull requests that straddle the change have been committed, (#1797, #1799) we can turn it off in the settings and push a commit to remove the configuration.

@jkbonfield
Copy link
Contributor Author

Ah yes I meant to update the htscodecs submodule and forgot.

I was assuming we'd just turn off the appveyor, rerun tests, and if it passed merge it. But we can do it in two separate PRs if really needed. It's minor fluff tbh.

@daviesrob daviesrob merged commit b8145e6 into samtools:develop Jul 4, 2024
9 checks passed
daviesrob added a commit to daviesrob/htslib that referenced this pull request Jul 8, 2024
Completes the migration of Windows testing to GitHub actions.
GitHub actions tests were added in commit 624e95b (PR samtools#1796).
daviesrob added a commit that referenced this pull request Jul 8, 2024
Completes the migration of Windows testing to GitHub actions.
GitHub actions tests were added in commit 624e95b (PR #1796).
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.

3 participants