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

CI: fix Windows build #126

Merged
merged 1 commit into from
Aug 16, 2024
Merged

Conversation

ctrueden
Copy link
Contributor

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.

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
Copy link
Contributor

cmhulbert commented Aug 15, 2024

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
Copy link
Contributor Author

@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
Copy link
Contributor

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!

@cmhulbert cmhulbert merged commit 19902d8 into saalfeldlab:master Aug 16, 2024
4 checks passed
@ctrueden ctrueden deleted the fix-windows-ci branch August 16, 2024 13:54
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