这是indexloc提供的服务，不要输入任何密码
Skip to content

Fix library name on Cygwin, MSYS and MinGW when using cmake #972

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 29, 2021
Merged

Fix library name on Cygwin, MSYS and MinGW when using cmake #972

merged 1 commit into from
May 29, 2021

Conversation

@mtl1979
Copy link
Collaborator

@mtl1979 mtl1979 commented May 22, 2021

See #971.

@codecov
Copy link

codecov bot commented May 22, 2021 •
edited
Loading

Codecov Report

Merging #972 (69d04de) into develop (c918062) will decrease coverage by 1.65%.
The diff coverage is n/a.

Impacted file tree graph

@@             Coverage Diff             @@
##           develop     #972      +/-   ##
===========================================
- Coverage    77.46%   75.81%   -1.66%     
===========================================
  Files           74       74              
  Lines         8313     8082     -231     
  Branches      1374     1345      -29     
===========================================
- Hits          6440     6127     -313     
- Misses        1339     1425      +86     
+ Partials       534      530       -4
Flag Coverage Δ
macos_clang 68.55% <ø> (ø)
macos_gcc 67.45% <ø> (ø)
ubuntu_clang ?
ubuntu_clang_debug ∅ <ø> (∅)
ubuntu_clang_inflate_allow_invalid_dist ?
ubuntu_clang_inflate_strict ?
ubuntu_clang_mmap ?
ubuntu_clang_msan ∅ <ø> (∅)
ubuntu_gcc 68.70% <ø> (-5.02%) ⬇️
ubuntu_gcc_aarch64 68.98% <ø> (-5.27%) ⬇️
ubuntu_gcc_aarch64_compat_no_opt 67.32% <ø> (-5.65%) ⬇️
ubuntu_gcc_aarch64_no_acle 67.89% <ø> (-5.40%) ⬇️
ubuntu_gcc_aarch64_no_neon 68.29% <ø> (-5.45%) ⬇️
ubuntu_gcc_armhf 68.97% <ø> (-5.28%) ⬇️
ubuntu_gcc_armhf_compat_no_opt 67.32% <ø> (-5.67%) ⬇️
ubuntu_gcc_armhf_no_acle 69.13% <ø> (-5.31%) ⬇️
ubuntu_gcc_armhf_no_neon 69.46% <ø> (-5.37%) ⬇️
ubuntu_gcc_armsf 68.98% <ø> (-5.28%) ⬇️
ubuntu_gcc_armsf_compat_no_opt 67.32% <ø> (-5.67%) ⬇️
ubuntu_gcc_compat_no_opt 68.71% <ø> (-5.48%) ⬇️
ubuntu_gcc_mingw_i686 0.00% <ø> (ø)
ubuntu_gcc_mingw_x86_64 0.00% <ø> (ø)
ubuntu_gcc_no_avx2 69.06% <ø> (-5.14%) ⬇️
ubuntu_gcc_no_pclmulqdq 66.88% <ø> (-5.24%) ⬇️
ubuntu_gcc_no_sse2 68.11% <ø> (-5.07%) ⬇️
ubuntu_gcc_no_sse4 66.85% <ø> (-5.25%) ⬇️
ubuntu_gcc_o3 68.43% <ø> (-5.17%) ⬇️
ubuntu_gcc_osb 70.98% <ø> (-5.55%) ⬇️
ubuntu_gcc_ppc 69.16% <ø> (-5.43%) ⬇️
ubuntu_gcc_ppc64 69.95% <ø> (-5.41%) ⬇️
ubuntu_gcc_ppc64le 68.65% <ø> (-5.44%) ⬇️
ubuntu_gcc_s390x 67.90% <ø> (-5.17%) ⬇️
ubuntu_gcc_sparc64 70.35% <ø> (-5.46%) ⬇️
win64_gcc 70.39% <ø> (ø)
win64_gcc_compat_no_opt 73.88% <ø> (ø)

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

Impacted Files Coverage Δ
zutil.c 37.50% <0.00%> (-44.65%) ⬇️
deflate.h 8.33% <0.00%> (-41.67%) ⬇️
trees.c 65.07% <0.00%> (-31.15%) ⬇️
trees_emit.h 67.53% <0.00%> (-27.53%) ⬇️
zutil_p.h 40.00% <0.00%> (-26.67%) ⬇️
arch/x86/compare258_avx.c 96.42% <0.00%> (-3.58%) ⬇️
test/fuzz/example_small_fuzzer.c 86.44% <0.00%> (-2.09%) ⬇️
arch/x86/adler32_avx.c 98.33% <0.00%> (-1.67%) ⬇️
test/fuzz/example_flush_fuzzer.c 88.70% <0.00%> (-1.62%) ⬇️
test/fuzz/minigzip_fuzzer.c 50.76% <0.00%> (-1.62%) ⬇️
... and 29 more

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 c918062...69d04de. Read the comment docs.

@nmoinvaz
Copy link
Member

nmoinvaz commented May 22, 2021

Doesn't this line in madler/zlib mean that it is expecting zlib1.dll for all WIN32?
https://github.com/madler/zlib/blob/cacf7f1d4e3d44d871b605da3b647f07d718623f/CMakeLists.txt#L208-211

@mtl1979
Copy link
Collaborator Author

mtl1979 commented May 22, 2021

@nmoinvaz I'm assuming UNIX is defined also when running Cygwin, MSYS and MinGW... All those have own names for the shared library. I can recheck the shared library name on hard disk of my old development PC, but it's too slow to compile code.

@mtl1979
Copy link
Collaborator Author

mtl1979 commented May 22, 2021

@nmoinvaz I changed the code, not sure if we want filename zlibstatic-ng.lib on Windows when compiling with Visual C++...

Open for suggestions as long as it doesn't cause import library of dll to have same name as static library.

@mtl1979
Only when using Visual C++, the static library name should be "zlibst…
69d04de 
…atic"

* On CygWin, MSYS and MinGW, the static library name should be "z" like on other Unix-like systems
nmoinvaz
nmoinvaz reviewed May 22, 2021
View reviewed changes
CMakeLists.txt
set_target_properties(zlib PROPERTIES OUTPUT_NAME zlib${SUFFIX})
endif()
# Static library
if(NOT DEFINED BUILD_SHARED_LIBS)
Copy link
Member

@nmoinvaz nmoinvaz May 22, 2021

Choose a reason for hiding this comment

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

Can you combine these four set_target_properties calls using ${ZLIB_INSTALL_LIBRARIES} as the target?

Copy link
Collaborator Author

@mtl1979 mtl1979 May 22, 2021

Choose a reason for hiding this comment

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

I don't think so... ${ZLIB_INSTALL_LIBRARIES} can also contain the shared library.

I tried combining matching ones using zlibstatic as target name but it did throw error that aliases can't be used with set_target_properties.

Copy link
Member

@nmoinvaz nmoinvaz May 25, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

What about using: z$<$<C_COMPILER_ID:MSVC>:libstatic>${SUFFIX}

Copy link
Collaborator Author

@mtl1979 mtl1979 May 25, 2021

Choose a reason for hiding this comment

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

That's pretty much unreadable to average developer...

@nmoinvaz
Copy link
Member

nmoinvaz commented May 22, 2021

I'm not quite convinced that we need this because madler/zlib doesn't use this right? What am I missing?

@mtl1979
Copy link
Collaborator Author

mtl1979 commented May 22, 2021

@nmoinvaz Stock zlib predates Cygwin, MSYS and MinGW, so its CMakeLists.txt wasn't fully tested with them. It used separate Makefile that did skip even configure.

@Dead2 Dead2 merged commit 2c65dce into zlib-ng:develop May 29, 2021
Dead2 added a commit that referenced this pull request Jun 11, 2021
@Dead2
Version 2.0.4
3fd9543 
- 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
@Dead2
Version 2.0.4
0388168 
- 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
@Dead2
Version 2.0.4
9302357 
- 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
Merged
Dead2 added a commit that referenced this pull request Jun 11, 2021
@Dead2
Version 2.0.4
558192b 
- 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

Reviewers

@nmoinvaz nmoinvaz nmoinvaz left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@mtl1979 @nmoinvaz @Dead2