search

saalfeldlab / n5

Not HDF5
BSD 2-Clause "Simplified" License
163 stars 22 forks source link

CI: fix Windows build #126

Closed ctrueden closed 2 months ago

ctrueden commented 3 months ago

This patch does three things:

  1. Consolidates the build-main and build-pr configs into one.

  2. Fixes the Windows build to actually do something, by adding the needed shell: bash lines into the .yml configuration.

  3. Skips the deployment for all platforms except Linux, to avoid double deploys / failing releases.

cmhulbert commented 2 months ago

Hey Curtis, thanks for the PR.

Looking into it, I actually think what we intended to do is only run the build-main on ubuntu-latest, which I think is why we didn't catch the lack of shell: bash causing the Windows builds to fail, since we already test against windows-latest in the platform-test workflow as well, and there it runs properly.

So I'm happy to accept this PR, but I'm wondering if you think there is any benefit to also running build-main on both ubuntu and windows, or if we can just remove the windows run from that workflow, and rely on platform-test for ensuring the tests are run on windows? That way, the deploy should only ever be called once anyway, on the ubuntu run

ctrueden commented 2 months ago

@cmhulbert Yeah, with the changes this PR makes, the platform-test.yml workflow is then redundant. My recommendation would be to remove the platform-test.yml in favor of a single build.yml that does the build + multi-platform testing. I personally prefer fewer workflows to reduce the overhead of spinning up additional transient containers, maintaining separate caches, etc.; since the tests naturally follow from the build (mvn test has to build things first, after all), it would just be more efficient.

But if y'all like the additional isolation of two workflows for some reason, you could then keep both... but right now, the main build leans on ci-build.sh, which also runs tests, and changing that would be a hassle. So then you'll end up running the tests twice, at least on Linux... unless you were to fork the ci-build.sh into this repo... Removing the platform-test.yml would be a lot simpler in my view.

cmhulbert commented 2 months ago

No, you make a good argument here, I'm happy to just remove the platform-test.yml for now, especially seeing as for N5 there is no special windows vs. linux logic. And even if so, I don't see any reason it couldn't be included in the build-main prior to calling the ci-build.sh step.

Alright, I'll merge this, and remove the platform.yml as well. Thanks for the PR, and taking to time to talk through it!