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 package config #412

Merged
merged 5 commits into from
Mar 12, 2021
Merged

Conversation

friendlyanon
Copy link
Contributor

Closes #411

This PR will componentize the install rules for the targets and install the CMake package config files as part of the dev component.

This should be an important enough change to notify package maintainers, not because this is a breaking change, but it makes separating the contents of the runtime (SONAME and REAL LIBRARY) and dev (NAMELINK, headers, etc) packages trivial.
Are there any who could easily be made aware of these changes?

@shibatch
Copy link
Owner

Is it possible to make this new installation strategy selectable with an option?

@friendlyanon
Copy link
Contributor Author

If CMake is not invoked with the --component argument in install mode, these changes don't affect anything, except the package config files are also installed.
By default CMake's install mode will install everything, you can only narrow the set of files being installed with the --component argument.

@shibatch
Copy link
Owner

Then there is nothing to worry about?
I think we don’t need to actively notify package maintainers about this.

@friendlyanon
Copy link
Contributor Author

What I meant by "notifying package maintainer" is just to let them know that the library now properly componentizes runtime and development files, which makes producing libx and libx-dev packages fairly trivial.

@shibatch
Copy link
Owner

Okay, so we anyway need to add good description on this functionality to the web site.
And we need some testing.

@friendlyanon
Copy link
Contributor Author

Sure, let me know what kind of description you'd like to see and how should the tests be implemented (assuming the tests should be part of this PR).

@shibatch
Copy link
Owner

Is there a project that is doing similar things?
I mean, description on the web page and testing.

@friendlyanon
Copy link
Contributor Author

I'm not aware, but these component changes are more for package maintainers to make the library easier to package, and the CMake package config is so users of the library can trivially consume it using find_package(sleef REQUIRED), which is the intended mechanic for using dependencies.

I have locally built the library and installed with no component + component arguments on Windows + MSVC, Ubuntu + Clang and Ubuntu + GCC. All files installed as they should.

If you mean test(s) for consuming the library yourself in CI, I have recently made an educational repository where I do just that.
As I see it now, the testing code is deeply embedded into the project code, so it consumes the library at a build tree level, i.e. you can't really test the library from an install location without big changes to project structure.

@shibatch
Copy link
Owner

This CMakeLists.txt is short enough.
Isn't it possible to do a similar thing to test if the installation is correct?

BTW, there is a simple testing on a nested build in the current CI setting.

stage('gcc-4.8 and cmake-3.5.1') {

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Feb 17, 2021

I see, you could make a separate CML, like in the example I linked, pack the project using cpack (CMake install mode was added in 3.15), install it use ninja install, then configure, build and run the test sub-project.

@shibatch
Copy link
Owner

Okay. Sounds good.

@friendlyanon
Copy link
Contributor Author

How should the testing for this be implemented? I was thinking that maybe you create the test code for this in a PR and I suggest changes about the CMake part? Or just take the existing test code for the "nested" CML and use that as well?

In the meantime, could you also please take a look at the comments I left on some of my changes to address things I'm uncertain about?

@shibatch
Copy link
Owner

I think it would be better if you could suggest a few lines of shell script code for testing.
Then, I will include it in the Jenkinsfile in a separate PR.

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Feb 18, 2021

In this CI workflow you can see what it takes to use a CMake package not installed to a system prefix.

For a system prefix it's a bit simpler:

# *nix platforms
mkdir -p build/root build/test
cd build/root
cmake ../.. -G Ninja # args
sudo cmake --build . --target install

cd ../test
cmake ../../test -G Ninja # args
cmake --build .
ctest

# Windows (MSVC)
md build\root
md build\test
cd build\root
cmake ..\.. # args
cmake --build . --config Release --target INSTALL

cd ..\test
cmake ..\..\test # args
cmake --build . --config Release
ctest -C Release

Of course these commands assume that you want to use older CMake versions where convenience switches for source and build directory were not present. You can see those switches in the above linked repository's CI workflow.

@shibatch
Copy link
Owner

Okay. Please go ahead.

@friendlyanon
Copy link
Contributor Author

OK, the PR is now ready for a complete review.

@friendlyanon friendlyanon marked this pull request as ready for review February 18, 2021 14:17
Previously inline_headers was a UTILITY target, which cannot be
installed, so it was renamed to inline_headers_util and a new
inline_headers INTERFACE library target is introduced instead.
@friendlyanon
Copy link
Contributor Author

Just a note, but this PR completely subsumes #247

@shibatch
Copy link
Owner

This patch looks good in terms of backward compatibility, but how can I check if the new functionality is working correctly?

@friendlyanon
Copy link
Contributor Author

With CMake 3.15+ it's trivial to see what is installed:

cmake --install build --component {sleef_Runtime | sleef_Development}

A different install_manifest_<component>.txt file is created when install mode is invoked with a component.

In older CMake versions, you can use components to instruct cpack to package components separately.

Is this what you were referring to?

@shibatch
Copy link
Owner

So is it enough to check whether install_manifest_.txt includes the files that are supposed to be there?

@friendlyanon
Copy link
Contributor Author

friendlyanon commented Feb 19, 2021

Here are the commands to generate the manifests for a specific component:

$ cpack -G TGZ -D "CPACK_INSTALL_CMAKE_PROJECTS=${PWD};sleef;sleef_Runtime;/" # CMake <3.15
CPack: Create package using TGZ
CPack: Install projects
CPack: - Install project: sleef []
CPack: Create package
CPack: - package: /home/vagrant/build/SLEEF-3.6.0-Linux.tar.gz generated.
$ cat install_manifest_sleef_Runtime.txt
/home/vagrant/build/_CPack_Packages/Linux/TGZ/SLEEF-3.6.0-Linux/lib/libsleef.so.3.6.0
/home/vagrant/build/_CPack_Packages/Linux/TGZ/SLEEF-3.6.0-Linux/lib/libsleef.so.3
/home/vagrant/build/_CPack_Packages/Linux/TGZ/SLEEF-3.6.0-Linux/lib/libsleefgnuabi.so.3.6
/home/vagrant/build/_CPack_Packages/Linux/TGZ/SLEEF-3.6.0-Linux/lib/libsleefgnuabi.so.3
$ tar ztvf SLEEF-3.6.0-Linux.tar.gz
drwxrwxr-x vagrant/vagrant   0 2021-02-19 15:30 SLEEF-3.6.0-Linux/lib/
-rw-r--r-- vagrant/vagrant 717256 2021-02-19 15:27 SLEEF-3.6.0-Linux/lib/libsleefgnuabi.so.3.6
-rw-r--r-- vagrant/vagrant 2456440 2021-02-19 15:28 SLEEF-3.6.0-Linux/lib/libsleef.so.3.6.0
lrwxrwxrwx vagrant/vagrant       0 2021-02-19 15:30 SLEEF-3.6.0-Linux/lib/libsleef.so.3 -> libsleef.so.3.6.0
lrwxrwxrwx vagrant/vagrant       0 2021-02-19 15:30 SLEEF-3.6.0-Linux/lib/libsleefgnuabi.so.3 -> libsleefgnuabi.so.3.6
$ cmake --install . --component sleef_Development --prefix ../install # CMake >=3.15
-- Install configuration: "Release"
-- Installing: /home/vagrant/build/../install/lib/cmake/sleef/sleefConfig.cmake
-- Installing: /home/vagrant/build/../install/lib/cmake/sleef/sleefConfigVersion.cmake
...
-- Installing: /home/vagrant/build/../install/lib/pkgconfig/sleef.pc
-- Up-to-date: /home/vagrant/build/../install/lib/libsleef.so

I wouldn't bother with checking them programmatically however.

Edit: I just noticed that sleefgnuabi does not have its VERSION property set to the full version number. Is that right?

@shibatch
Copy link
Owner

Okay, because you don't think that checking the content of install_manifest_<component>.txt is necessary, I won't add it.

As for the version property of sleefgnuabi, I have to ask @fpetrogalli.
If he does not respond, we don't need to change it.

@friendlyanon
Copy link
Contributor Author

Hmm, well even I pointed out that the REAL LIBRARY is only for humans, I guess then it does not matter that the suffix doesn't contain the full version, it's just the SONAME that the runtime loader will go looking for on *nix platforms.

I just felt like pointing out that the file meant for humans does not contain the complete version number.

@friendlyanon
Copy link
Contributor Author

Been a while. Things are ready for a complete review from you.

@shibatch
Copy link
Owner

shibatch commented Mar 8, 2021

Sorry, but I'm a bit busy in other tasks.
I will do it in this week.

@shibatch
Copy link
Owner

I tested with several configurations.
It seems OK to merge.

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

Successfully merging this pull request may close these issues.

CMake package?
2 participants