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

cmake-remake #1027

Open
wants to merge 100 commits into
base: develop
Choose a base branch
from
Open

cmake-remake #1027

wants to merge 100 commits into from

Conversation

Vollstrecker
Copy link

Hi,

I guess I have everything together. A quick overview:

  • the second template for zconf.h is gone, you can adjust everthing in one place from where all is created
  • it's now possible to build minizip
  • all undocumented options are gone
  • zlib and minizip provide (currently activated by default) copies of some libs for compat with the old naming, this is to be dopped somewhen in the future as both provide cmake-configs and pkgconfig files so every automated build should pick the right names. If there are handcrafted solutions out there with hardcoded names, it's time to update as this won't work for the next 100 years.

The tests on dll based plattforms are mostly run with only the static libs as dll has no rpath option. I also aded a runner file for cygwin and msys. cygwin succedes after restarting the job, but not on the first one, so I renamed the file. I can't reproduced that locally even when using the commands from the runner. Maybe someone with more knowledge in these systems and/or yml can improve this part.

For minizip there are currently no tests run as the would need codechanges that are not subjected here. There are c++-style comments that aren't allowed here and warnings ab out data loss when converting _uint64 to long here. Also I wasn't able to find the right symbol to export the api and had to tell windows to export them all. Maybe someone has more insight here also.

Some questions are still open:
#116 also set's ZLIB_WINAPI, when is this needed?
#72 adds contrib/masmx64/inffas8664.c, if so, when should this be added?
#539 How often will it happen to have semicolon in you dir names?

So I guess these 3 could be closed later by hand.

For #983 I can't say anything as I've never crosscompiled, maybe @trcrsired can check on that

So here's the list of stuff that's done:
Closes: #43 #132 #133 #138 #148 #162 #170 #177 #218 #219 #265 #271 #277 #315 #337 #344 #359 #366 #432 #444 #471 #472 #489 #526 #543 #545 #558 #581 #713 #718 #721 #762 #781 #816 #818 #831 #835 #849 #899 #966 #976 #1009 #1026

In addition there are some that are fixed but not closed before:
Closes: #242 was closed can be closed again
Closes: #259 because it's possible with cmake
Closes: #270 vc15 not needed anymore
Closes: #306 because Makefile.msc is gone already
Closes: #428 symbol is not found anywhere
Closes: #575 was fixed before
Closes: #569 because Makefile.msc is gone already
Closes: #600 duplicate of #162
Closes: #652 because there was a merged fix
Closes: #860 done but not closed
Closes: #972 seems to be a usage error

For the future I also plan cpack integration for neat creation of installers, and to look at everything that's not included (examples/contrib). If there occur any problems I could offer you to open the issue-tracker on the cmake-remake fork to keep track there (not my first choice as users would need to know) or that you highlight me or the project (no clue if this is possible) to support the future of zlib from that side.

Vollstrecker added 30 commits November 30, 2024 16:19
add option to also install zlib1.ddl for compat
partially superseeds madler#976
adjust min required version for regex, superseeds madler#43
@Vollstrecker Vollstrecker force-pushed the develop branch 3 times, most recently from 0c83ea3 to 91c8f61 Compare December 31, 2024 13:27
@Vollstrecker
Copy link
Author

I guess I'm done - with some confusion

First: the thing with minizip and bzip2, I would have expected to get some errors about missing symbols from the compiler when HAVE_BZIP2 is set, but it seems none of the tests is using this and I don't get any errors when not linking against it.

Should a hard dependency on bzip2 be added anyways, or should it stay like described in the README?

Second: The cygwin workflow almost always fails at the first run but succeeds when restarted. As it fails on different places during the compile stage I suppose some random segfault on cygwins side.

Should this test be disabled until someone that knows the system better can fix this?

@Vollstrecker
Copy link
Author

For all with an opinion on this topic, cmake 3.10 as minimum is old I know that, but I don't miss any feature of newer versions that are needed. If you really need anything for newer versions that doesn't work in this one, now is the time to complain.

The question in this case is just what should be supported. If Ubuntu LTS 20.04 is still a thing, 3.16 would be possible. If Ubuntu LTS 22.04 is the magic boundary, we could jump to 3.22. Newer than 3.25 I wouldn't like to go as then at least Debian stable will still work.

And yes, as the workflows get newer version and every time this happens an older version will be deprecated, this minimum will increase over time and newer methods will then also be included for the new minimum when applicable.

@madler
Copy link
Owner

madler commented Dec 31, 2024

@Vollstrecker One check failed. (mingw/cygwin / Cygwin)

@Vollstrecker
Copy link
Author

@Vollstrecker One check failed. (mingw/cygwin / Cygwin)

Yes, I mentioned that as the second point. Mostly if fails on the first and succreds when restartet. I asked if I should leave it in as it seems something like a segfault happens.

@Vollstrecker
Copy link
Author

Forgot to mention: While minizip compiles fine on mingw32 which reports 32Bit integers, I wasn't able to get the same with -m32 on gcc. Maybe someone can take a look on that also.

@Neustradamus
Copy link

@Vollstrecker: Good job!

About minizip -> @gvollant, he does not answer quickly badly.

@Neustradamus Neustradamus mentioned this pull request Jan 1, 2025
@ClausKlein
Copy link

ClausKlein commented Jan 2, 2025

Looks good for me.

2 Notes about cmake code style:

  • It is not consistent formatted
  • The endblocks should be (<empty>) IMHO

And some cmake variables are uninitialized:

CMake Warning (dev) at CMakeLists.txt:160 (set_source_files_properties):
  uninitialized variable 'CMAKE_RC_OUTPUT_EXTENSION'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:169 (add_library):
  uninitialized variable 'ZLIB_DLL_SRCS'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:180 (target_compile_definitions):
  uninitialized variable 'MSVC'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:180 (target_compile_definitions):
  uninitialized variable 'MSVC'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:217 (target_compile_definitions):
  uninitialized variable 'MSVC'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:217 (target_compile_definitions):
  uninitialized variable 'MSVC'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at CMakeLists.txt:225 (set_target_properties):
  uninitialized variable 'zlib_static_suffix'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at test/CMakeLists.txt:150 (add_test):
  uninitialized variable 'PLATFORM'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at test/CMakeLists.txt:186 (add_test):
  uninitialized variable 'PLATFORM'
This warning is for project developers.  Use -Wno-dev to suppress it.

CMake Warning (dev) at test/CMakeLists.txt:222 (add_test):
  uninitialized variable 'PLATFORM'
This warning is for project developers.  Use -Wno-dev to suppress it.

@ClausKlein
Copy link

ClausKlein commented Jan 2, 2025

Some typos should be checked/fixed later on:

./gzread.c:117: wit ==> with
./test/example.c:408: hel ==> help, hell, heal
./contrib/minizip/CMakeLists.txt:176: VERSIOM ==> VERSION
./contrib/minizip/zip.c:425: clen ==> clean, clan
./contrib/minizip/zip.c:433: clen ==> clean, clan
./contrib/testzlib/testzlib.c:163: compres ==> compress, compares
./examples/gzappend.c:180: readin ==> reading, read in
./examples/gzappend.c:194: readin ==> reading, read in
./examples/gzappend.c:350: readin ==> reading, read in
./examples/gzjoin.c:121: bload ==> bloat
./examples/gzjoin.c:140: bload ==> bloat
./examples/gzjoin.c:194: bload ==> bloat
./examples/gzjoin.c:257: bload ==> bloat
./examples/gzlog.c:299: clen ==> clean, clan
./examples/gzlog.c:423: clen ==> clean, clan
./examples/gzlog.c:445: clen ==> clean, clan
./examples/gzlog.c:691: clen ==> clean, clan
./examples/gzlog.c:696: clen ==> clean, clan
./examples/zran.h:24: re-usable ==> reusable
./doc/rfc1950.txt:121: implementors ==> implementers
./doc/rfc1951.txt:140: implementors ==> implementers
./doc/rfc1951.txt:209: uncompressible ==> incompressible
./doc/rfc1951.txt:780: implementor ==> implementer
./doc/rfc1952.txt:127: implementors ==> implementers

@Vollstrecker
Copy link
Author

2 Notes about cmake code style:

* It is not consistent formatted

Without more details it's only guessing. I added cmake-format.yaml file and let it do the the job.

* The endblocks should be `(<empty>)` IMHO

Including this stuff in the endblocks was mandatory in the early days and can be omitted nowadays. However especially on longer or nested statements it's much easier to see where you are. It's like in any header where you have ifdef foo ... endif #foo

And some cmake variables are uninitialized:
uninitialized variable 'ZLIB_DLL_SRCS'

This one was overlooked and should be gone earlier. For the others, I've never seen anyone initializing cmake-internal vars, as it's cmake's task. However the way cmake works with vars makes sure that uninitialzed ones resolve to an exmpty string and as they are checked for a specific value to take effect, it's totally save to use them that way.

For the typos, The VERSIOM was my bad, that's fixed. For the others, as this is only about cmake I'm way beyond focus right now with fixing configure and Makefile.in thanks to the Ubuntu-transition in the runners, and I have a feeling that this is some unchecked automated list. I'm no expert, but local int reading, read in(file *in) doesn't look to me like something I want to have in C.

@ClausKlein
Copy link

ClausKlein commented Jan 3, 2025

I found just a problem with minizip.

see cmake-remake@e7e9e81

bash-5.2$ ctest --rerun-failed --output-on-failure
Test project /Users/clausklein/Workspace/c/zlib/build-x86_64-Debug/standalone
    Start 14: minizip_find_package_configure
1/2 Test #14: minizip_find_package_configure ...***Failed    0.89 sec
-- The C compiler identification is Clang 19.1.6
-- Detecting C compiler ABI info
-- Detecting C compiler ABI info - done
-- Check for working C compiler: /usr/local/opt/llvm/bin/clang - skipped
-- Detecting C compile features
-- Detecting C compile features - done
-- Configuring done (0.8s)
CMake Error at /Users/clausklein/Workspace/c/zlib/build-x86_64-Debug/standalone/test/test_install/lib/cmake/minizip/minizip-shared.cmake:61 (set_target_properties):
  The link interface of target "MINIZIP::minizip" contains:

    BZip2::BZip2

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /Users/clausklein/Workspace/c/zlib/build-x86_64-Debug/standalone/test/test_install/lib/cmake/minizip/minizipConfig.cmake:53 (include)
  CMakeLists.txt:10 (find_package)


CMake Error at /Users/clausklein/Workspace/c/zlib/build-x86_64-Debug/standalone/test/test_install/lib/cmake/minizip/minizip-static.cmake:61 (set_target_properties):
  The link interface of target "MINIZIP::minizipstatic" contains:

    BZip2::BZip2

  but the target was not found.  Possible reasons include:

    * There is a typo in the target name.
    * A find_package call is missing for an IMPORTED target.
    * An ALIAS target is missing.

Call Stack (most recent call first):
  /Users/clausklein/Workspace/c/zlib/build-x86_64-Debug/standalone/test/test_install/lib/cmake/minizip/minizipConfig.cmake:53 (include)
  CMakeLists.txt:10 (find_package)


-- Generating done (0.0s)
CMake Generate step failed.  Build files cannot be regenerated correctly.

    Start 15: minizip_find_package_build
Failed test dependencies: minizip_find_package_configure
2/2 Test #15: minizip_find_package_build .......***Not Run   0.00 sec

0% tests passed, 2 tests failed out of 2

Total Test time (real) =   1.00 sec

The following tests FAILED:
	 14 - minizip_find_package_configure (Failed)
	 15 - minizip_find_package_build (Not Run)
Errors while running CTest
bash-5.2$ 

@Vollstrecker
Copy link
Author

minizip doesn't link against bzip2, so there shouldn't be any mention of it in the import. That's why I asked this as my first first confusion.

@ClausKlein
Copy link

fixed with cmake-remake@0802d85

@ClausKlein
Copy link

ClausKlein commented Jan 4, 2025

Maybe we have to set minimum CMake version to 3.16 caused by CMP0095

and use CMAKE_INSTALL_RPATH to install minizip right.

@craigscott-crascit May you have a look at cmake-remake@e826338 ?

@Vollstrecker
Copy link
Author

The last commit seems all OK, just the version looks a bit to high. Yes I tested with 1.4.1.1 but I'm sure it wont be found atm.

For he rpath, I don't see how it would help, But I can't check anything for the next 2 days, at least.
If someone adds rpath support for windows, I would do anthing I can to help.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment