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

Please do not install irrelevant cmake modules #163

Open
yurivict opened this issue Apr 7, 2022 · 18 comments
Open

Please do not install irrelevant cmake modules #163

yurivict opened this issue Apr 7, 2022 · 18 comments

Comments

@yurivict
Copy link

yurivict commented Apr 7, 2022

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
Copy link
Collaborator

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

@yurivict
Copy link
Author

yurivict commented Apr 7, 2022

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

@fbergmann
Copy link
Owner

As described in the numl post: NuML/NuML#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
Copy link

shoops commented Apr 8, 2022

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
Copy link

shoops commented Apr 8, 2022

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
Copy link

shoops commented Apr 8, 2022

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
Copy link
Author

yurivict commented Apr 8, 2022

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
Copy link

shoops commented Apr 8, 2022

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
Copy link
Owner

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
Copy link
Author

yurivict commented Apr 8, 2022

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
Copy link
Owner

could you suggest a namespace to use then?

@yurivict
Copy link
Author

yurivict commented Apr 8, 2022

It should be under its library name - LibSBML.

@fbergmann
Copy link
Owner

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
Copy link
Author

yurivict commented Apr 8, 2022

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
Copy link

shoops commented Apr 8, 2022

I think here are two different topics messed up:

  • Where to install:
    1. share/cmake/Modules
    2. share/NAME/Modules
    3. share/combine/Modules

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.

  • How do we call the targets. We have 2 restrictions:
    1. aaaaa::bbbbb
    2. either aaaaa or bbbbb must be the find modules name

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
Copy link

shoops commented Apr 8, 2022

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
Copy link
Author

yurivict commented Apr 8, 2022

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
Copy link

shoops commented Apr 8, 2022

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.

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

4 participants