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 build would delete the source file zconf.h #781

Closed
fredgan opened this issue Feb 17, 2023 · 8 comments
Closed

CMake build would delete the source file zconf.h #781

fredgan opened this issue Feb 17, 2023 · 8 comments

Comments

@fredgan
Copy link

fredgan commented Feb 17, 2023

Hi, @madler
The CMake build would rename zconf.h to zconf.h.included.

$ git status
On branch master
Your branch is up to date with 'origin/master'.

Changes not staged for commit:
  (use "git add/rm <file>..." to update what will be committed)
  (use "git restore <file>..." to discard changes in working directory)
        deleted:    zconf.h

Untracked files:
  (use "git add <file>..." to include in what will be committed)
        build/
        zconf.h.included

Mostly this would not be a quesion. But sometimes, some build rules or system would require that source file should not be changed after the build.

The git blame shows that the rename part zconf.h in CMakeLists.txt is changed 10 years ago. Perhaps this part can be optimized.

 69 if(NOT CMAKE_CURRENT_SOURCE_DIR STREQUAL CMAKE_CURRENT_BINARY_DIR)
 70     # If we're doing an out of source build and the user has a zconf.h
 71     # in their source tree...
 72     if(EXISTS ${CMAKE_CURRENT_SOURCE_DIR}/zconf.h)
 73         message(STATUS "Renaming")
 74         message(STATUS "    ${CMAKE_CURRENT_SOURCE_DIR}/zconf.h")
 75         message(STATUS "to 'zconf.h.included' because this file is included with zlib")
 76         message(STATUS "but CMake generates it automatically in the build directory.")
 77         file(RENAME ${CMAKE_CURRENT_SOURCE_DIR}/zconf.h ${CMAKE_CURRENT_SOURCE_DIR}/zconf.h.included)
 78   endif()
 79 endif()
@jfern2011
Copy link

jfern2011 commented Mar 18, 2023

I'm running into this same issue. When I #include "zlib.h" I get a compiler error that zconf.h is missing, so the workaround is to symlink zconf.h to zconf.h.included

@RandallFlagg
Copy link

I had the same problem. It was solved by adding the following line before linking:
target_include_directories(myapp PRIVATE "${CMAKE_CURRENT_BINARY_DIR}/${ZLIB_LIBRARY}")

I am not sure if I solved it correctly but it would be great if this can be simplified in the future.

My project tree is as follows:

myapp
-|_ zlib

@fredgan
Copy link
Author

fredgan commented Feb 18, 2024

Do you think the part (Line69~79) can be deleted? @madler @jfern2011 @RandallFlagg?

@madler
Copy link
Owner

madler commented Feb 18, 2024

Do you think the part (Line69~79) can be deleted? @madler @jfern2011 @RandallFlagg?

The solution, whatever it is, will need to be more elegant than that. It is there to fix someone's workflow. If I just delete it, then their's will be broken and yours might be fixed. It appears to be difficult to concoct a CMakeLists.txt that will make everyone happy.

@fredgan
Copy link
Author

fredgan commented Feb 20, 2024

Do you think the part (Line69~79) can be deleted? @madler @jfern2011 @RandallFlagg?

The solution, whatever it is, will need to be more elegant than that. It is there to fix someone's workflow. If I just delete it, then their's will be broken and yours might be fixed. It appears to be difficult to concoct a CMakeLists.txt that will make everyone happy.

Thanks for your reply. You're exactly right. Build scripts affect most users.

But in my opinion, breaking most people's source code in order to consider someone's using their own zconf.h for compilation doesn't seem so reasonable. In fact, they have other ways to solve this problem without having to customize CMakeLists, such as replace their zconf.h to the source code or using their own CMakeLists for compilation, etc.

What's more, my company has many products use zlib while no one use their own zconf.h for compilation. Probably very few people use this feature.

@sadtab
Copy link

sadtab commented Oct 29, 2024

+1 For @fredgan, I would suggest externalizing this whole logic in a cmake file and enable it only if custom zconf.h is present

@TheNicker
Copy link

Hi @madler,
First and foremost I would like to thank you on the great work here

Do you think the part (Line69~79) can be deleted? @madler @jfern2011 @RandallFlagg?

The solution, whatever it is, will need to be more elegant than that. It is there to fix someone's workflow. If I just delete it, then their's will be broken and yours might be fixed. It appears to be difficult to concoct a CMakeLists.txt that will make everyone happy.

It only breaks for others if an upstream update takes place.

Breaking changes are acceptable. it's very difficult to make progress without it. the idea is to keep the balance by when and how much to break, too little and you get stagnant, too much and users run away.

It's legitimate to pave a new path, no one is forced to follow, the current version remains unchanged by definition.

This Bug in the CMake script is just an easily fixable annoyance.
"the perfect is the enemy of the good" in a non utopian perfect world. so let's try to do some good

@madler
Copy link
Owner

madler commented Feb 2, 2025

See #1027 .

@madler madler closed this as completed Feb 2, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

6 participants