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

workflow: add MacOS compile check #12696

Merged
merged 1 commit into from
Jul 22, 2024
Merged

Conversation

wenduwan
Copy link
Contributor

@wenduwan wenduwan commented Jul 19, 2024

Increase CI coverage to prevent #12693

@wenduwan wenduwan force-pushed the test_macos_arm branch 7 times, most recently from 60aad92 to 894b472 Compare July 19, 2024 01:11
@wenduwan wenduwan changed the title TEST MACOS INTEL workflow: add MacOS ARM compile check Jul 19, 2024
@wenduwan wenduwan requested review from jsquyres and bosilca July 19, 2024 01:14
@wenduwan
Copy link
Contributor Author

FYI @dalcinl I copied your setup script from https://github.com/mpi4py/mpi-publish/blob/master/.github/workflows/cd-wheel.yml#L126

I believe our licenses are compatible.

@dalcinl
Copy link
Contributor

dalcinl commented Jul 19, 2024

@wenduwan Why did you call the workflow file compile-macos-intel.yaml ? You should remove the -intel suffix.

@wenduwan wenduwan changed the title workflow: add MacOS ARM compile check workflow: add MacOS compile check Jul 22, 2024
@wenduwan
Copy link
Contributor Author

Thanks @dalcinl and @jsquyres . Based on what I'm hearing, I think what we really want is to test on the latest macOS, regardless of the underlying architecture. This simplifies the workflow a bit.

Also added make parallelization as Lisandro suggested.

@wenduwan wenduwan requested review from dalcinl and jsquyres July 22, 2024 14:48
@dalcinl
Copy link
Contributor

dalcinl commented Jul 22, 2024

@wenduwan Is there any quick make check or make test that you could run (after a make install) to make sure that the thing you just built is actually usable at runtime?

@wenduwan
Copy link
Contributor Author

@dalcinl Good question. I think we might need to run something close to the mpi4py suite in order to have some reasonable coverage.

@jsquyres I wonder what should be our "minimal" coverage on mac?

@jsquyres
Copy link
Member

@wenduwan Wouldn't be a bad thing to do a make check (which runs a bunch of unit tests) and maybe then also a make in the examples directory and does an mpirun -np NUM_CORES on each of the executables (hello world and ring in the different bindings).

I wouldn't worry too much about make distcheck -- we test that in Linux, and a) that's a good indicator that it will (probably) also work on MacOS, and b) we realistically rarely invoke make distcheck on MacOS anyway.

Increase CI coverage to prevent open-mpi#12693

Signed-off-by: Wenduo Wang <[email protected]>
@wenduwan wenduwan requested a review from jsquyres July 22, 2024 19:38
Copy link
Member

@jsquyres jsquyres left a comment

Choose a reason for hiding this comment

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

It only runs a single ring_c test, but that's good enough for now (i.e., it's good enough as a sanity test). Let's merge this now and add more macos testing over time.

@wenduwan wenduwan merged commit a24343d into open-mpi:main Jul 22, 2024
14 checks passed
@wenduwan wenduwan deleted the test_macos_arm branch July 22, 2024 19:52
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants