CMake and Zlib #831
Comments
|
Other people linked to CMake and Zlib. @malytomas, @PiotrSikora, @avoidik, @jeking3, @JonasVautherin, @sav-ix, @01e9, @henryborchers, @zdenop, @rgpublic, @Slackadays, @bear24rw, @kgerheiser, @paulsapps, @danielalves, @rocammo, @bmarques1995, @rafael-santiago, @Andreas-Schniertshauer, @Togtja, @joeyparrish, @robiwano, @jeking3, @henryborchers, @aneeshpb-amazon, @kavionic, @cloudwindy |
|
There are too many cmake issues and pull requests for me to go through and disposition them, and I wouldn't know what's good or bad anyway. I very much like @Mizux 's suggestion to have a group render on, coordinate, and most importantly verify proposed cmake changes. My main concern are proposed changes that work in one case, but messes up a bunch of others. Ideally this group would be able to cover a good set of platforms. |
|
You might want to give interested parties Triage access to the repository to help manage all the GitHub issues. You could decide to merge in PRs based on the number of GitHub code review approvals. I’d say stick to only merging changes that are absolutely necessary or ones that fix problems that lead to many duplicate issues. |
|
It would help a lot if someone with the necessary rights could approve the CI for PRs. Mine has been pending for 1+ year with no test run. |
|
My |
|
@ClausKlein Neat workaround for the blocked CI. |
|
@ClausKlein: Nice :) I hope it will be a good beginning! @madler, can you look? |
|
@vszakats: Can you look improvements from @ClausKlein to adapt your PR? |
|
@Neustradamus Both PRs touch the buggy Can't see anything else I could adapt from the other PR, they are targeting different issues. UPDATE: to my RC comment above: missed a |
|
I see this diff on my branch to #667, should I apply it? diff --git a/CMakeLists.txt b/CMakeLists.txt
index d4eb7cb..19ac18b 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -121,7 +121,7 @@ set(ZLIB_SRCS
zutil.c
)
-if(NOT MINGW AND NOT MSYS)
+if(WIN32)
set(ZLIB_DLL_SRCS
win32/zlib1.rc # If present will override custom build rule below.
)
diff --git a/win32/Makefile.gcc b/win32/Makefile.gcc
index 081e391..35172fb 100644
--- a/win32/Makefile.gcc
+++ b/win32/Makefile.gcc
@@ -50,7 +50,7 @@ AR = $(PREFIX)ar
ARFLAGS = rcs
RC = $(PREFIX)windres
-RCFLAGS = --define GCC_WINDRES
+RCFLAGS =
STRIP = $(PREFIX)strip
diff --git a/win32/zlib1.rc b/win32/zlib1.rc
index ceb4ee5..5253b6a 100644
--- a/win32/zlib1.rc
+++ b/win32/zlib1.rc
@@ -1,11 +1,7 @@
#include <winver.h>
#include "../zlib.h"
-#ifdef GCC_WINDRES
VS_VERSION_INFO VERSIONINFO
-#else
-VS_VERSION_INFO VERSIONINFO MOVEABLE IMPURE LOADONCALL DISCARDABLE
-#endif
FILEVERSION ZLIB_VER_MAJOR,ZLIB_VER_MINOR,ZLIB_VER_REVISION,0
PRODUCTVERSION ZLIB_VER_MAJOR,ZLIB_VER_MINOR,ZLIB_VER_REVISION,0
FILEFLAGSMASK VS_FFI_FILEFLAGSMASK |
|
I think we are better keeping PRs granular and each fixing distinct problems, so I'd keep them separate. I'm also fine with rebasing if necessary. This makes it easier to later bisect where a problem was introduced or track down what changed and why. Our issue IMO is that none of the CMake patches are actually merged. (Unless we want to somehow offer one "megapatch" solving all issues at once, which is possible but needs a great amount of work and testing.) |
As far as I know, |
|
My patch (#677) fixes that by deleting all |
|
|
CMake documented it in 3.2 (2015-03-04) indeed, but zlib's use predates that: dc5a43e (2011-09-09) by almost 4 years. Even more interestingly, the first introduction of
|
MSYS2 still seems to define I agree that there is bugs in some versions of cmake when it comes to relying to correct variables getting defined. Some projects manually define these and not rely on cmake defining them automatically. |
|
Current zlib solution for RC compilation is a toolchain-specific hack, and broken. Question: Was it purposefully done that way to support ancient CMake versions? Was it an oversight? CMake does support Also, are the resource optimization flags targeting Windows 16-bit (3.11 from 1992-04-06) intentionally kept there or is it okay to delete them yet? It'd be useful to get answers or any kind of feedback instead of guessing and pushing PRs that are perhaps wrong or not desired by zlib. This would help getting out of the CMake-stall we experience. CMake being the only standard tool to build zlib with, this also seems critical. |
|
@mtl1979: mingw-64 (and old mingw32) (= IMO we should deal with those if an actual fallout is reported and not optimize for niche tool bugs upfront. I've personally found these macros working as expected. |
cmake uses preprocessor to detect the operating system as seen on the patch posted above. |
It seems correct to me. This is also the initial commit for this feature, so it doesn't tell the full story. Anyhow I don't to seem get where this discussion is heading, so closing up my PR and will continue to patch the zlib build I maintain. |
|
pip doesn't replace anything installed in I also don't understand what |
zlib is supposed to support 16-bit and 32-bit operating systems... In recent days the support for 64-bit operating systems (and larger compressed files) has gotten better and more complete. |
|
@mtl1979: Re-read what I wrote, or look at the patch. This concerned I'm back to the less obtrusive workaround for the bug I'm hitting. #347 would also help because |
|
Maybe some people can look for this ticket: |
|
I wonder when people will start drawing the obvious conclusions about CMake. Of course there are many justifications to its use like being locked in or having no power or say, but it is clearly requiring enormous amounts of time and energy investments to just use it properly (in all its edge cases). At some point the madness has to stop. |
CMake does have a lot of limitations but it's still somewhat easier to maintain than manually writing configure scripts... People have not used autoconf or automake in ages even though those were meant to help with creating configure scripts and Makefiles. |
Whichever is easier is to an extent a matter of opinion, but definitely a matter of a developer's experience. My criticism was directed towards CMake, not to suggest that the GNU Autotools are the best solution for zlib. But I can comment on that. There are objective reasons why the infamous GNU Autotools are easier, for example one can decide to what degree to use them (generated or static Makefiles, generated config.h or not, free choice of other template files). So, there is a gentle learning curve compared to GNU Makefiles. Also, it builds on useful or already present knowledge, like shell scripting and said GNU Makefiles. There is a reason that the GNU Autotools are still widely used, but I see projects that offer support for one or more additional build systems. You do realize that zlib is currently using something that is identical to the GNU Autotools, right? configure, Makefile template, header file templates. For minizip even Automake is used. CMake is a very different story as any developer with enough experience can confirm. However, there is no need to convince anyone -- regardless CMake's supposed strengths or benefits we can see that support for CMake is a problem for zlib, hence this very GitHub issue. |
|
My 2 cents on this is that CMake and Autotools are pretty much orthogonal to each other, and not supporting CMake would leave a very large community hanging. And it isn't really rocket science. |
Thanks. I once met a rocket scientist who claimed that rocket science is not rocket science :-P I would claim that CMake is at least more frustrating than rocket science. How would you describe the current situation? Is CMake currently supported? That is why I suggested 2 solutions:
For people in favor of CMake the first option is obviousy most attractive. For the second option to work well, zlib needs to cooperate. However, now we have neither. In the end, the current developers decide, but anyone is free to fork... |
|
CMake is not rocket science, but I can attest that being a rocket scientist does not help with understanding CMake. configure was not auto-generated — it is hand-written. zlib will continue to support both configure/make and cmake. At least to the extent that anyone considers the current CMakeLists.txt to be supporting cmake. No, this is not the place to debate whether 'tis better to cmake or not to cmake.
So who can be the judge of whether any given change is actually an improvement or not? It's not me. I don't just apply whatever commits willy nilly. Anyone can propose a change to CMakeLists.txt that fixes their local situation, but also screws up someone else's. Recently I reverted a CMakeLists.txt commit for that reason. In my opinion, we would need cmake testing across a wide variety of platforms, beyond what is available in github actions. |
|
@madler: I understand that. Judging contributions isn't an easy feat. Perhaps one way to bootstrap this it is to merge the most reasonably solid-looking PRs, and/or to request peer-reviews for the most important ones. Or to require adding CI tests before merge. This would also assume that CI tests are actually allowed to run (manually or by default), because at the moment this isn't the case. Adding CMake integration tests would also help, e.g. like this one. Another option is to ask for as granular PRs as possible, targeting one specific issue at a time. While also separating style/format changes from logic changes. I understand that all of this is work and not necessary a small amount, but zlib being probably the most exposed dependency out there, there is a good chance to distribute this among many interested contributors. |
|
@madler wrote:
Please forgive me if I've misunderstood you, but does this mean that it is your position as maintainer that CMakeLists.txt is frozen now and will not change? This is an important detail for those of us who have issues using the existing cmake support for one reason or another. |
I use three different CI providers myself, GitHub Actions is one of them. Besides Linux, MacOS and Windows, I can test pretty much any operating system that Google has CI image for, but mostly I just test different FreeBSD versions. |
No. Note the "At least" which you left out of your emphasis. |
|
Because there are many projects out there that have the same issues with their cmake, there is a new project that tries to help. zlib is the first project that could benefit. atm. I sorted and mostly implemented the pending pull request from here and then the issues will be worked out before reworking the rest to fit into modern times without going over the edge just to use a fancy new feature. As for now the resources are limited I want to ask a) all the people here to wave a hand if they can test all the stuff on systems github has no runner for and/or b) to join and take an active part either for the rework of zlib and/or everything that will come in the future of https://github.com/cmake-remake Tests will be implemented for linux, mac, windows, mingw (all flavours) and cygwin. Everyone that is able to test on other systems (also on these) is very welcome. |
|
@Vollstrecker That's a good idea and thanks for taking the plunge. The litmus test is going to be able to come forward to new zlib releases with first-class cmake support out of the box. Our main dependencies are via libpng and the boost iostreams support for streamed gzip compression. |
|
@Vollstrecker Thank you! This sounds like exactly what I have been asking for. Please let me know when you think it's ready for prime time. |
|
Sure, you'll get a PR as soon as the tests are done. FYI: I took the freedom to change the created lib to be named z on all platforms including windows. Static libs have a s suffix (zs.dll) and debug-builds get d appended (zd.dll/zsd.dll). As the export will create all targets to link against, noone will notice. For old stuff zlib1.dll is copied from z.dll. The copy can be deactivated via option but I'm not sure if it should better default to OFF. Let me know if this is a bit too much for a start, or if something is unclear what I mean. |
|
@Vollstrecker Would it be possible to make the static suffix customisable and/or tweak it for MinGW? For MinGW specifically it's unexpected to use |
|
This get's more complicated, but if this was the case in the past, this needs to be preserved. |
|
Would be nice to have optional overrides for all the static and shared library targets. |
|
Sorry, but this a not a good reason. If you use zlib-ng you most likely use cmake. Then you just do find_package(zlib) and link against ZLIB::ZLIB or ZLIB:ZLIBSTATIC and don't have to care what the real name is. Btw. zlib-ng pretents to be compatible drop-in, so any name zlib had in the past should also be supported by them. So the zlib-ng naming you already tied to should be the zlib naming like it was the past 10+ years. The compat copy was intended for people that have a VS-project that has the old name hardcoded. If there was a different naming on mingw, then this will also be in the reworked version. This was never intended to give everyone the possibility to make up new names. I'm sure noone wants reports about failing compiles against a.dll b.dll c.dll just to find out at the end that a and c are 2 different versions of the same lib. |
|
I might be mis-remembering what was needed across our stack for zlib-ng. I'm confident your revamp here will get us at least 95% to cmake nirvana. (Nirvana = It "just works" without hacking at third party cmake) Perhaps our complications are of our own making, due to a lot of static linking and a lot of things linking to zlib. |
In general there no guarantee that someone will use CMake to build a project depending on a CMake-built zlib. There is also no guarantee that a dependent project's CMake build will use cmake-config, or Speaking of MinGW, The actual lib filenames do matter in a significant portion of the cases. (With zlib-ng this happens to work without issues with MinGW.) edit: For debug libs, CMake has a built-in variable edig 2: Speaking of MSVC (= non-MinGW) the static suffix becomes an issue
PR → cmake-remake#1 |
|
There are some other useful patches that we have to apply to, which maybe you'd be willing to consider:
|
Not instantly, but it's a long term goal.
I first focused on Linux and Win, for sure everything that worked in the past has to work later also. I don't know how MinGW decides which one to choose if both are present, but I also don't really care.
And it will work with zlib aswell. But to be honest, zlib-ng is a different project and even if you had something to change to work with that, you can't expect that to still work here because it's a different project that does other cruel stuff. But I'm not here to talk about that.
Yep, and again I already have it in there and just didn't push. For 2 libs it doesn't really makes a big difference, but there is some stuff in contrib/ that needs that, too.
Your'e confusing suffix and postfix. The postfix get's appended to the filename, the suffix is the actual extension. On MSVC (or plain Win in general) this is dll for both, that's why the postfix is there.
You mean like appending a d to libname or something like that? I'm pretty sure that is exactly what I'm doing.
This will apply for handcrafted projects (most commonly VS solutions) and I see 2 situations for this.
These 2 only occur on Windows as for any other system out there nothing changes at all.
Not that I wouldn't welcome useful PRs, but I don't like work in progress interrupts. But I can go through to clarify some points: L83: Breaks on cygwin For the rest: For me zlib1.dll is nothing for ever. If Windows would support it I would just symlink this. Somewhen in the future the compat will default to off and somewhen later it will disappear. Yes, this can take 10+ years. It's nothing unusual that things change over time. If someone adopted the old cmake stuff here, it's no problem to adopt the new stuff also. I someone changed to zlib-ng, it's no problem to stay there or to change back. If someone builds the traditional way, it's no problem to just continue this. I can fully understand every maintainer to not rework such stuff, as almost every time someone pops up with additional wishes and more time is used to discuss these than to do actual work on that. If something breaks, everyone can open an issue and it will get fixed. I want to use my old stuff the new way without updating is no issue. I'm going to test this on as many platforms as I can and I'm going to also compile some other stuff against it. If you have examples that get problems, feel free to list them so I can check that, but please don't criticize work that's only half way done yet. |
Both are appended to the output filename, this particular Your draft force-adds a 3rd, initial suffix before the above postfix.
You're doing it for both debug and static, also unconditinally
That makes this effort pointless IMO. The point would be to enable zlib CMake
Easy to fix by extending the condition to CYGWIN/MSYS. Though as said there that edit: on a second look: the The MINGW error:
There is nothing black magic in it. It's checking if the two output
I'm not criticizing your work, but raising issues that I had found in And apparently no, nobody can open an issue and get anything fixed, I think any renaming effort should be separated from the actual CMake My 2 cents. |
The suffix is (on Win) .dll, and you are setting it to be .dll. So you try to postfix the name by prefixing the suffix. And yes, I'm adding a 3rd level by marking static builds and debug builds. As you know i.e. MSVC is able to produce debug and release from one config created by cmake, so you can either make sure that all have different names, or you would have to configure different install-dirs to select between debug or not. This way you can have all in one with just one run of cmake.
Here you write it: Force a different name. z.dll vs. zs.dll. But here's the trick: If you build an installer, it's able to see that another installer already installed zlib and therefor doesn't need to install another copy. I know this is new and pure magic for Windows users, but it works. But it only works if the lib is named the same, so if the other app that installed zlib decided the want it named different, the installer sees in the registry that there's an installation and skips this part, but the app is not able to find it. There's no benefit (at least noone showed me a valid reason atm.) in allowing to change that, but huge potential that it makes things worse.
If they want to stick with their old method, even the first introduction of cmake here was pointless. If someone wants to use cmake, then it should be done right. In this case right means cmake switched from find_package with modules to find_package with configs (for a reason) and so should everyone that wants to use cmake. The module takes care of everything and provides a target to use. In this case it's completely irrelevant how the lib is named. Problems only occur if they use find_package to make sure zlib is there (maybe with altered names to search for) and then use the name instead of the target in the call to link_libraries. In this case everyone should realize that not only code needs maintenance, abuild-system does also.
Sure, but do you know where else it is needed. I would prefer to decide based on COMPILER_ID but for whatever reason this is not implemented for RC. It's possible that it's behind one of the others and I just don't know because I'm not a windows guy. If so, let me know. As you can see in cmake's source they set the .obj based on the filename of rc_compiler, so I guess they also don't rely on the platform-var for reason and by now this is the only way to see if windres is used somewhere else.
That's code decisions I wont judge upon.
Not on my local, not pushed yet, code. I don't understand why this is there at all (no windows guy). But it seems to me that it is something for dll's to be used in windows and as it's possible to compile against libs in cygwin from outside of cygwin, I guess it's needed there also. And as I don't get any error so far, I'm pretty sure it's not completely wrong to include it there also.
You use ZLIB_STATIC_LIB_SUFFIX which is no cmake var and not set on any system I'm testing atm. (linux/windows/mingw32/mingw64/cygwin) so I guess it's supposed to be set via -D on cmdline (or something similar) but not created with option to be documented. It's an hidden option that can be found when reading the source and therefor I consider it black magic.
You're raising issues without pointing them out. I see "would be nice to allow" and there can be problems without pointing out which ones. I you say problems with curl, then I can check if it works without any modification to their code, that's more helpful than everything I've seen before.
As you may have noticed, there are many things missing there atm. I didn't test, but I didn't deactivate issues so they should work. But yes, issues would distract from the work itself. I'm currently trying to sort all the pr's (so they can be referenced) identify what's duplicated or does the same in different ways, then adressing the issues already reported. Getting issues in that phase for half done work just slows this process down. Not to mention that this project should not be the place to report issues as it's not the projects repo. Imagine there comes a pr that fixes an issue reported but the maintainer doesn't know about it. Issues should always be associated with the project and not with an outside contributor, regardless if he intends to maintain that stuff in the future or not. As this is the first attempt under the new project, I still need to figure out the best way to do things. In this case I tried to let everyone know that there is someone that addresses all the issues and come to the conclusion it's better for the future to work silently until the PR is ready and discuss problems there (as they belong there).
I try to retain compat with an option that is on by default. I don't know the complete history of zlib1.dll, but it seems that there was an api-change and no other way to make sure to get the right one. As it's the same with every workaround, they just stay there. With cmake there is a proper way to ask for specific versions, so there's no need for this workaround, it just stays there to ease migration but is clearly marked as a temporary solution. The renaming shouldn't cause any problems. If they do I'm happy to fix that if it's not based on other workarounds. If there are conventions on platforms like it seems for MinGW, then they have to be implemented (and will be after I found the docs about it).
Noted. |
There are already options to choose from. ZLIB_BUILD_STATIC=OFF gives you only the shared one. Your patch relies on CMAKE_BUILD_SHARED_LIBS which makes it impossible to get both in one run. If the static suffix is needed if only static is built is debatable, but this causes problems with you don't see what it is. I'm sure vcpkg users are able to just link against ZLIB::ZLIBSTATIC if the want have only a static link.
The first half is already there (not sure if pushed already) the second half shoudn't be a problem. Is it just not working on android or is this for some crosscompiling? I'm asking because I guess if it's for crosscompiling it's better to check for rc compiler existence to not get the same problem with other combinations.
Isn't that what CMAKE_FLAGS_INIT is meant for?
I remember problems with missmatch of these and I'm not near enough to this world to understand these options but iirc the problems came from mixing variants that should be sorted out with using the right targets to link. |
They end up as separate C runtimes. You can't link two objects built against the different types. You have to use extreme caution when crossing DLL boundaries (e.g. memory pointers can cross, but don't try to free() in one place when it was malloc()'ed in the other, and don't ever pass a FILE pointer between the two). |
This can be extended to linking zlib both statically and dynamically on same project... In almost all cases, dynamically linked version should be used. Visual C++ gives no guarantee which library each symbol is resolved from, so there is chance that you get same symbol in zlib itself and another library that use zlib internally. Depending on version used, they might internally use different memory allocation functions. |
@Vollstrecker, you can find my contact at https://joeyparrish.github.io/. I can test builds for you if you need me to. I have both x64 & arm64 Macs, x64 Windows, and both x64 & arm64 Linux. And one of my best friends has a huge collection of ancient Unix machines of various flavors and architectures, and I can easily bribe him to let me SSH in if you need me to test a build on something exotic. I usually respond to email within a week, more often within a day or two. |
|
@Adenilson, @bahaa-cpl: About your PR: [mini_zip][build] Add minizip build target: cc: @gvolant. |
|
To be honest, I'm just doing exactly that. I'm currently investigating a warning about a conversion __int64 to long in ioapi.cL190 which I don't get fixed. About the fact that @madler doesn't want it to be included, the CMakelists.txt in there will work completely independet from the main one, so it can be called on it's own, but there is an option (default to OFF) that it can be included in the main-build. |
|
@Vollstrecker I'm on summer holidays now, so I look a minute to give this new CMakeLists.txt a go on Ubuntu 24.04 amd64. Worked fine for me, has some needed options such as optional shared and static libraries. There might be some odds and ends that I'd follow up with later, such as optional -fPIC and checking that the exported symbols are minimal. But thumbs up, this is at least 99% of what zlib needs for wrangling as a long-term dependency of various things. Cheers! |
@madler: It is possible to solve all problems and merge some PRs?
Thanks in advance.
Issues:
--
ZLIB::ZLIBfor CMake. zlib-ng/zlib-ng#1320PRs:
I have not finished:
Linked to:
@Mizux, @albrematt, @johnb003, @miurahr, @WildCard65, @wrowe, @ScatteredRay, @jnhyatt, @Jihadist, @hwhsu1231, @mtl1979, @ClausKlein, @sum01, @craigscott-crascit, @hjmallon, @mattparks, @puneetmatharu, @vszakats, @tbeu, @fredgan, @RobinGeffroy, @jfern2011, @kirankotari, @etiennearnal, @omegacoleman, @yyzworker, @yalcinerbora, @niparx, @navidR, @lperron, @vguen, @tr1cks, @AndrewAtAvenza, @gvollant, @hopeless-programmer-online, @glebm, @jheaff1, @SpaceIm, @AraHaan, @pzychotic, @siepkes, @schultetwin1, @Bagira80, @wantehchang, @dankegel, @chanphil, @MichaelGoulding, @stkruse, next after.
@nmoinvaz
The text was updated successfully, but these errors were encountered: