jasper-software / jasper

Official Repository for the JasPer Image Coding Toolkit
http://www.ece.uvic.ca/~mdadams/jasper
Other
217 stars 103 forks source link

CMake: Avoid conditional lib names based on generators #350

Closed SpaceIm closed 1 year ago

SpaceIm commented 1 year ago

Generators are an internal detail of users, lib name shouldn't depend on it. Currently a d postifx is added to debug lib name in case of multi config generator. It makes lib name & packaging content unpredictable for conan package manager for a given tuple of compiler/os/arch/build type.

jubalh commented 1 year ago

What would your proposed change look like?

SpaceIm commented 1 year ago

To remove this block: https://github.com/jasper-software/jasper/blob/402d096b39f4f618ad9e6ff2b4fc1b5eb260a2e4/CMakeLists.txt#L266-L268

SpaceIm commented 1 year ago

Actually in jasper recipe of conan-center, we even disable this JAS_MULTICONFIGURATION_GENERATOR branching.

jubalh commented 1 year ago

@theta682 you added this in https://github.com/jasper-software/jasper/commit/0f020fd7. Unfortunately your commit message is quite unhelpful.

But I see that in https://github.com/jasper-software/jasper/pull/301 you actually wrote:

    Make CMake code style similar to standard CMake
    Improve multi-configuration support: Add "d" suffix for Debug configuration
    Improve detection of snprintf on Windows
    Use BUILD_SHARED_LIBS to declare JAS_ENABLE_SHARED if it is defined
    Reduce warning about GLUT on Windows

Please do one change per commit in the future and describe it.

Anyways, the PR mentions Improve multi-configuration support: Add "d" suffix for Debug configuration. @theta682 what do you think about @SpaceIm suggestion here?

theta682 commented 1 year ago

In the case of "multi-configuration" debug suffix is required. CPack which uses CMake install will replace all configurations with the last one. There is no way to package both configurations at once. For preparing a separate debug package it is better to use nonmulti-configuration. I am strongly against removing the debug suffix when multi-configuration is used.

mdadams commented 1 year ago

@jubalh: Based on the comments from @theta682, I would be inclined to leave JasPer unchanged in the above regard. Should I close this issue?

SpaceIm commented 1 year ago

This makes things harder for package managers (we would have to patch forever in conan just because a user has a very specific and unusual usage of the build system of library :s ). I never seen such weird name logic in any CMake based project. Debug suffix based on compiler, yes (but honestly it's a pain), but it doesn't make sense when it depends on generator.

theta682 commented 1 year ago

@SpaceIm if you package using multi-configuration you must support multiple files. Don't use multi-configuration generator.

theta682 commented 1 year ago

After more thought, I can agree with @SpaceIm that CMAKE_DEBUG_POSTFIX can be provided externally. See #351.

jubalh commented 1 year ago

Thanks @theta682 for the patch and @SpaceIm for the discussion.

mdadams commented 1 year ago

Thank you all @theta682 @SpaceIm @jubalh