search

giaf / blasfeo

Basic linear algebra subroutines for embedded optimization
Other
321 stars 88 forks source link

Cmake: define a installer package #139

Open ThijsWithaar opened 3 years ago

ThijsWithaar commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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 commented 3 years ago

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.