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: define a installer package #139

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

ThijsWithaar
Copy link

This defines a cmake package, such that 'make package' will create an package or installer which can be removed from the system.

It also fixes a linker error (multiple definitions of BLASFEO_PROCESSOR_FEATURES)

@imciner2
Copy link
Contributor

imciner2 commented Mar 2, 2021

The two bugfix commits look fine to me (and thanks for fixing the linker error - I just saw the issue that points that out in the code I wrote a while ago). I am not convinced that the CPack parts of the CMake are a good idea though. What is wrong with doing a make install and using a separate deb creation script? Every use of the CPack I have seen has always been debian specific it seems, which is not going to be universal (I personally use RPMs on Fedora).

@ThijsWithaar
Copy link
Author

ThijsWithaar commented Mar 2, 2021

The CPack is definitely something which is not widely popular. Personally, I use both the NSiS (windows), .deb and .rpm archives fairly regularly, with just the 'package' target. For me, the main benefit is having a clean uninstall of whatever test version I want to try out

It's of course your project, so feel free to pick only the other fixes.

@giaf
Copy link
Owner

giaf commented Mar 5, 2021

First of all, thanks for the PR and the bug fixes.

Regarding CPack, I would also be in favor of keeping the cmake as general as possible.
I'm not super skilled in cmake: is there an "unistall" rule, similarly to the "make install" one?

@ThijsWithaar
Copy link
Author

As far as I know, there's no uninstall in cmake. For me personally, having it as package means the system package manager can always uninstall it, even if the source archive changes/is removed.

Would it be better if I strip out the debian specific lines (that's 1168 to 1177) ?
That would keep the required definitions to make CPack happy and still keep it general.

@giaf
Copy link
Owner

giaf commented Mar 5, 2021

Would it be better if I strip out the debian specific lines (that's 1168 to 1177) ?

I think this sounds as a good compromise.
What happens by removing these lines on a debian system?

Is there a specific reason for choosing blasfeo-dev in here?
set( CPACK_PACKAGE_NAME "blasfeo-dev")
This PR will end up in the master branch of BLASFEO.

@imciner2
Copy link
Contributor

imciner2 commented Mar 5, 2021

As far as I know, there's no uninstall in cmake.

It is fairly easy to add an uninstall target to CMake, and it actually exists in the Acados CMake so I could port it to the BLASFEO CMake with little trouble.

@giaf
Copy link
Owner

giaf commented Mar 5, 2021

It is fairly easy to add an uninstall target to CMake, and it actually exists in the Acados CMake so I could port it to the BLASFEO CMake with little trouble.

If it makes the job for everyone, this would be my preferred solution too.
At the end the un-installation should be rather straightforward (i.e. simply remove the library and the entire header folder) so there should be little risk of getting it wrong also in case the BLASFEO repo changes in the mean while.

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.

3 participants