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

Fix compiler detection to avoid bad mpicc match #959

Merged
merged 2 commits into from
May 18, 2021

Conversation

gsjaardema
Copy link
Contributor

@gsjaardema gsjaardema commented May 13, 2021

Redo of #956. The previous test would match the MPI mpicc with the test for matching the intel compiler icc. This test uses CMake determined CMAKE_C_COMPILER_ID which is a safer test.

Also fixes a test for the host machine. APPLE was missing CMAKE_HOST_APPLE

@gsjaardema
Copy link
Contributor Author

Sorry, should have checked before assuming, but APPLE is a valid CMake variable, so does not need to be changed to CMAKE_HOST_APPLE. I can remove that change if desired.

@nmoinvaz
Copy link
Member

It does not need APPLE at all because UNIX implies APPLE. So it would be better if APPLE was removed.

@nmoinvaz
Copy link
Member

LGTM thank you!

@gsjaardema
Copy link
Contributor Author

I think a maintainer has to click a button to allow the workflows to run for this PR. I am a first-time contributor to this repository.

@nmoinvaz
Copy link
Member

@Dead2 will need to approve.

@Dead2
Copy link
Member

Dead2 commented May 16, 2021

Did anyone actually test this with an Intel compiler to verify we don't have any regressions?

@codecov
Copy link

codecov bot commented May 16, 2021

Codecov Report

Merging #959 (0ea7b59) into develop (54b1c13) will decrease coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #959      +/-   ##
===========================================
- Coverage    77.36%   77.34%   -0.02%     
===========================================
  Files           74       74              
  Lines         8309     8309              
  Branches      1374     1374              
===========================================
- Hits          6428     6427       -1     
  Misses        1348     1348              
- Partials       533      534       +1     
Flag Coverage Δ
macos_clang 68.55% <ø> (ø)
macos_gcc 67.45% <ø> (ø)
ubuntu_clang 73.61% <ø> (ø)
ubuntu_clang_debug 73.65% <ø> (ø)
ubuntu_clang_inflate_allow_invalid_dist 73.34% <ø> (ø)
ubuntu_clang_inflate_strict 73.60% <ø> (ø)
ubuntu_clang_mmap 73.31% <ø> (ø)
ubuntu_clang_msan 73.61% <ø> (ø)
ubuntu_gcc 73.00% <ø> (ø)
ubuntu_gcc_aarch64 74.48% <ø> (-0.02%) ⬇️
ubuntu_gcc_aarch64_compat_no_opt 73.32% <ø> (ø)
ubuntu_gcc_aarch64_no_acle 73.62% <ø> (-0.03%) ⬇️
ubuntu_gcc_aarch64_no_neon 74.08% <ø> (ø)
ubuntu_gcc_armhf 74.48% <ø> (-0.02%) ⬇️
ubuntu_gcc_armhf_compat_no_opt 73.31% <ø> (ø)
ubuntu_gcc_armhf_no_acle 74.67% <ø> (-0.02%) ⬇️
ubuntu_gcc_armhf_no_neon 75.07% <ø> (ø)
ubuntu_gcc_armsf 74.49% <ø> (-0.02%) ⬇️
ubuntu_gcc_armsf_compat_no_opt 73.31% <ø> (ø)
ubuntu_gcc_compat_no_opt 74.42% <ø> (ø)
ubuntu_gcc_mingw_i686 0.00% <ø> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <ø> (ø)
ubuntu_gcc_no_avx2 74.52% <ø> (ø)
ubuntu_gcc_no_pclmulqdq 71.46% <ø> (ø)
ubuntu_gcc_no_sse2 72.54% <ø> (ø)
ubuntu_gcc_no_sse4 71.42% <ø> (ø)
ubuntu_gcc_o3 73.79% <ø> (ø)
ubuntu_gcc_osb 75.98% <ø> (ø)
ubuntu_gcc_ppc 74.80% <ø> (ø)
ubuntu_gcc_ppc64 75.56% <ø> (ø)
ubuntu_gcc_ppc64le 74.32% <ø> (ø)
ubuntu_gcc_s390x 73.23% <ø> (ø)
ubuntu_gcc_sparc64 76.03% <ø> (ø)
win64_gcc 70.39% <ø> (ø)
win64_gcc_compat_no_opt ?

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files Coverage Δ
arch/arm/adler32_neon.c 97.50% <0.00%> (-2.50%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 54b1c13...0ea7b59. Read the comment docs.

@gsjaardema
Copy link
Contributor Author

@Dead2 Yes, I have tested it with CC=icc cmake .. and with CC=mpicc cmake .. (where mpicc is based on an intel compiler) and both correctly identify that the compiler (or underlying compiler) is intel-based. Configuration suceeds; build succeeds and tests complete.

@Dead2 Dead2 merged commit bde11b1 into zlib-ng:develop May 18, 2021
@gsjaardema gsjaardema deleted the fix-compiler-test branch May 18, 2021 14:50
@scivision
Copy link
Contributor

I tested this on Windows as well. Before this PR, GCC mpicc fails, and with this PR, it's fixed. Thank you!

Dead2 added a commit that referenced this pull request Jun 11, 2021
- Minor code cleanup #983 #984
- Fix mpicc compilation #959
- Fix build on NetBSD #964
- Fix build on OpenBSD #970
- Fix build on Cygwin #972 #974
- Fix linter warnings in configure #975
- Spelling fixes #961
- Improve unistd.h handling #960
- Remove stdarg.h detection #976
- CI/Test improvements #977 #981 #985
- Cmake improvements #980
Dead2 added a commit that referenced this pull request Jun 11, 2021
- Minor code cleanup #983 #984
- Fix mpicc compilation #959
- Fix build on NetBSD #964
- Fix build on OpenBSD #970
- Fix build on Cygwin #972 #974
- Fix linter warnings in configure #975
- Spelling fixes #961
- Improve unistd.h handling #960
- Remove stdarg.h detection #976
- CI/Test improvements #977 #981 #985
- Cmake improvements #980 #989
Dead2 added a commit that referenced this pull request Jun 11, 2021
- Fix inflate corruption #982
- Minor code cleanup #983 #984
- Fix mpicc compilation #959
- Fix build on NetBSD #964
- Fix build on OpenBSD #970
- Fix build on Cygwin #972 #974
- Fix linter warnings in configure #975
- Spelling fixes #961
- Improve unistd.h handling #960
- Remove stdarg.h detection #976
- CI/Test improvements #977 #981 #985
- Cmake improvements #980 #989
@Dead2 Dead2 mentioned this pull request Jun 11, 2021
Dead2 added a commit that referenced this pull request Jun 11, 2021
- Fix inflate corruption #982
- Minor code cleanup #983 #984
- Fix mpicc compilation #959
- Fix build on NetBSD #964
- Fix build on OpenBSD #970
- Fix build on Cygwin #972 #974
- Fix linter warnings in configure #975
- Spelling fixes #961
- Improve unistd.h handling #960
- Remove stdarg.h detection #976
- CI/Test improvements #977 #981 #985
- Cmake improvements #980 #989
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.

None yet

4 participants