fbergmann / libSEDML

SED-ML library based on libSBML
BSD 2-Clause "Simplified" License
9 stars 12 forks source link

Please do not install irrelevant cmake modules #163

Open yurivict opened 2 years ago

yurivict commented 2 years ago

2.0.30 installs:

share/cmake/Modules/FindBZ2.cmake
share/cmake/Modules/FindCHECK.cmake
share/cmake/Modules/FindEXPAT.cmake
share/cmake/Modules/FindLIBNUML.cmake
share/cmake/Modules/FindLIBSBML.cmake
share/cmake/Modules/FindLIBXML.cmake
share/cmake/Modules/FindXERCES.cmake
share/cmake/Modules/FindZLIB.cmake

that aren't used by the project.

luciansmith commented 2 years ago

I think they are? libsedml depends on libsbml, its tests are run with check, and the rest are libsbml's dependencies.

yurivict commented 2 years ago

When you install such modules - random other packages might pick them up instead of what they are using and fail for this reason.

fbergmann commented 2 years ago

As described in the numl post: https://github.com/NuML/NuML/issues/26, changing to use find scripts was the only way to ensure that the export scripts dont absolute paths of the build machine.

These find scripts are there, so that the imported targets would work, no matter what configuration of libSBML was used as a starting point.

shoops commented 2 years ago

When you install such modules - random other packages might pick them up instead of what they are using and fail for this reason.

This is incorrect if a package has their own FindXXXX module that would be somewhere in their package and that location would have to be added to the CMAKE_MODULE_PATH. This module is preferred to the standard module.

shoops commented 2 years ago

I would prefer to only install FindSEDML. LibSBML should install FindLIBSML and all necessary modules for its configuration, i.e., if it is linked against expat FindEXPAT. If it is campiled with zlib support FindZLIP, etc.

shoops commented 2 years ago

I am strongly for providing those modules out off own interest so that we not have to maintain FindSBML multiple times. As a compromise we might consider to install those modules in PREFIX/share/combine/Modules.

yurivict commented 2 years ago

Ideally SBML should install its own cmake module so that all its dependent packages should reuse it.

Dependencies shouldn't be duplicating FindSBML or installing modules for packages that SBML depends on.

shoops commented 2 years ago

Ideally SBML should install its own cmake module so that all its dependent packages should reuse it.

Dependencies shouldn't be duplicating FindSBML or installing modules for packages that SBML depends on.

I agree

fbergmann commented 2 years ago

ok, then i will next week change all the libraries again, to do it that way. LibSBML, will install the find scripts it needs, together with the find script for libsbml. All other libraries, will adjust their CMAKE_MODULE_PATH to include the one installed by libSBML, and then just install their own find module into that directory.

yurivict commented 2 years ago

LibSBML, will install the find scripts it needs

These should be installed into its own namespace, not globally, to prevent interaction with other packages.

fbergmann commented 2 years ago

could you suggest a namespace to use then?

yurivict commented 2 years ago

It should be under its library name - LibSBML.

fbergmann commented 2 years ago

I'm still not clear about this. Are you suggesting that instead of installing find scripts like ZLIB::ZLIB you want them to be LibSBML::ZLIB? Because that goes contrary to everything i thought of doing. Even cmake distributes a FindZLIB script with a ZLIB::ZLIB target, and i tried to fashion all the scripts like it, so we can have all the libraries we need.

So i'm against prefixing the dependent libraries with libSBML, as for example COPASI will need the expat target as well. So EXPAT::EXPAT is the better one to use there.

yurivict commented 2 years ago

I am not a cmake expert, but files that you install should only affect LibSBML and not third party software. So installing generic cmake scripts shouldn't be done.


expat, check and xerces-c3, for example. provide .pc files, so you can just use them.

shoops commented 2 years ago

I think here are two different topics messed up:

Only the first choice which is the default for CMake modules may present a possible conflict if the module name is already existing. The other 2 are fully under our control. If we are not using 1) I would vote for 3) as all other libraries numl, combine, libsedml, etc can be installed in that place. I personally suggest 1) for libraries which we control like libsbml, libcombine, libnuml, and libsedml since we are the authority. These would make them accessible to all 3rd parties. For support libraries like libxml2, expat, raptor or zlib I suggest that libsbml installs them under: share/libsbml/Modules (option 2). The only modules which needs to find these targets is FindSBML and it know its installation directory.

For libraries we control we should use NAME::NAME where the associated CMake module is called FindNAME.cmake. For libraries which we do not control and for which no CMake modules are available. I suggest that we use the following as NAME: AAAAA_BBBBB where AAAAA is our library e.g. SBML and BBBBB would be EXPAT, RAPTOR, XML2 or ZLIB. This would lead to module names in share/libsbml/Modules like FindSBML_EXPAT.cmake or FindSBML_RAPTOR.cmake.

shoops commented 2 years ago

expat, check and xerces-c3, for example. provide .pc files, so you can just use them.

I think you are not understanding the problem. We still need a module appropriately named FindEXPAT.cmake, which will look for it. If that does not exist we have to write our own. We have done more than once our intention is to avoid copying the code.

yurivict commented 2 years ago

We still need a module appropriately named FindEXPAT.cmake [...]

cmake supports pkg-config files through pkg_check_modules. So there's really no need for FindXx.cmake for them.

shoops commented 2 years ago

cmake supports pkg-config files through pkg_check_modules. So there's really no need for FindXx.cmake for them.

We are of course aware of it and we are using it in our modules as one alternative to find library information. However, our experience is that relying on pkg-config files to be present as the only source of information is not sufficient. What works on Linux or BSD does not necessarily work under Windows.