About missing libinstpatch.pc

[jjceresa]

I am currently test CMake for generating projects files build by VS. When building the project named "INSTALL", this installs files (binaries: dll, libraries: .lib, and includes: .h) in the directory indicated by CMAKE_INSTALL_PREFIX option.

Typically, this directory is intended to be used as a dependency when building other application (e.g swami,...) through CMake. In this case, i guess libinstpatch.pc should be generated by INSTALL. Unfortunately it seems that libinstpatch.pc isn't generated.

Please, did you get similar behaviour on linux when invoking "make install" ?

[derselbst]

libinstpatch-1.0.pc is currently only generated on UNIX or when using mingw:

if ( UNIX OR MINGW )
    # pkg-config support
    set ( prefix "${CMAKE_INSTALL_PREFIX}" )
    set ( exec_prefix "\${prefix}" )
    set ( libdir "\${exec_prefix}/${LIB_INSTALL_DIR}${LIB_SUFFIX}" )
    set ( includedir "\${prefix}/${INCLUDE_INSTALL_DIR}" )
    configure_file ( libinstpatch-1.0.pc.in
        ${CMAKE_BINARY_DIR}/libinstpatch-1.0.pc IMMEDIATE @ONLY )
    install ( FILES ${CMAKE_BINARY_DIR}/libinstpatch-1.0.pc
              DESTINATION ${LIB_INSTALL_DIR}${LIB_SUFFIX}/pkgconfig )
endif ( UNIX OR MINGW )

This should be fixed, you are correct.

Also, I would like to remove the version from the pkgconfig file, i.e. renaming it to libinstpatch.pc just like we do for fluidsynth, which seems to be the recommend way to do. We may keep the major version though (libinstpatch-1.pc) to allow installing multiple version of this library, but I don't see a use-case for this.

[jjceresa]

This should be fixed, you are correct.

Thanks for confirming. To be honest i haven't yet take time to learn how to write Cmake instructions. I should do so.

Also, I would like to remove the version from the pkgconfig file, i.e. renaming it to libinstpatch.pc just like we do for fluidsynth. We may keep the major version though (libinstpatch-1.pc) to allow installing multiple version of this library, but I don't see a use-case for this.

libinstpatch.pc, should be sufficient, as far the version value should be explicitly indicated inside this file. i guess this is automatically checked by Cmake against the required version isn't ?

[derselbst]

i guess this is automatically checked by Cmake against the required version isn't ?

Yes, it is.

[jjceresa]

Also, in CMakelists.txt files a lot of name aren't coherent leading to confusion concerning target version. I would to fix this. For example: 1) add_library ( instpatch-1.0.... The target name instpatch-1.0 shouldn't be hard coded but stamped by the current package version. This could be changed like this: set ( TARGET_NAME libinstpatch-${IPATCH_VERSION_MAJOR}.${IPATCH_VERSION_MINOR}) then add_library ( ${TARGET_NAME}.... This is important because it impacts the name final executable (ie: libinstpatch-1.1.dll, libinstpatch-1.1.lib) 2) install ( FILES ..... DESTINATION include/libinstpatch-1.0/libinstpatch ) could be changed install ( FILES ..... DESTINATION include/ ${TARGET_NAME}/libinstpatch )

And so on for any instpatch-1.0 hard coded in any CMakelists.txt files. The only exception could be for pkgconfig filename which could belibinstpatch.pc. Any opinions ?

[derselbst]

If we decide to rename the pkgconfig file to libinstpatch.pc we will also have to remove the version suffix from library filenames, i.e. calling them just libinstpatch.dll.

[jjceresa]

Having binary library files ( .lib static, .lib import, .dll) without explicit version information is very confusing for further link use and require they are installed in different directory to be uniquely identifiable. At least on Windows there is a close relation at build time for the couple (.lib import library, .dll) generated. Later when an application is intended to use with a this dll linked at load time, this application must be linked with the appropriate import library at built time. Otherwise this could lead to a failed link at load time, or a possible violation access at execution time. It I would be preferable to keep these major.minor suffixes in .lib,.dll and perhaps in .pc files . In all cases it seems to me that any suffixes in .pc file name brings no useful information.

[derselbst]

I understand your point. We can add the major version as suffix to the filename on windows, just as we do in fluidsynth. However, I don't see the point in adding the minor version as well. Breaking changes that change ABI and cause access violations when linking require a major version bump. Thus I would vote to only suffix the major version.

[jjceresa]

Breaking changes that change ABI and cause access violations when linking require a major version bump. Thus I would vote to only suffix the major version.

This seems good and sufficient. This should lead to major version suffix choice for:

• cmake target name
• binaries target file name (eg, .lib ,.exe, .dll)
• installed includes sub directories name.

In fluidsynth pkg-config is fluidsynth.pc without suffix, i guess you agree with libinstpatch.pc too ?.

[derselbst]

i guess you agree with libinstpatch.pc too ?.

Yes.

[jjceresa]

Ok, i will propose a PR to address all these target naming comments.