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

Switch to cmake-suggested install path variables #36

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

merkys
Copy link

@merkys merkys commented Apr 20, 2021

I noticed the project uses *_INSTALL_DIR variables to control installation destinations. CMake recommends using CMAKE_INSTALL_*DIR variables, as "[T]his allows package maintainers to control the install destination by setting the appropriate cache variables." I am packaging libemf2svg for Debian, and having CMAKE_INSTALL_*DIR variables would be more convenient to do that.

@kakwa
Copy link
Owner

kakwa commented Apr 20, 2021

Hello,

Thanks for the PR.

However, I have two concerns regarding this change:

  1. the cmake debian helper seems to cope ok with the existing install variables, I've in fact already packaged libemf2svg (though my packaging is probably not compliant with Debian policies), and the rule file didn't need any tweak, https://github.com/kakwa/amkecpak/blob/master/libemf2svg/debian/rules, the resulting packages are available here (https://mirror.kakwalab.ovh/deb.sid/raw/). However, there are indeed some warnings in the build logs regarding these variables.
  2. libemf2svg is also packaged by other distributions (Suse and Alpine Linux), and I don't want to risk breaking there packaging
    (https://build.opensuse.org/package/view_file/graphics/libemf2svg/libemf2svg.spec?expand=1). Also, it doesn't seem the cmake macro used by RPM supports the CMAKE_INSTALL* variables: https://src.fedoraproject.org/rpms/cmake/blob/rawhide/f/macros.cmake.

If this change really do make things easier/more compliant, would it be possible for you to rework it so that both *_INSTALL_DIR and CMAKE_INSTALL_*DIR variables work?

This issue also has been discussed in the past in #22

@merkys
Copy link
Author

merkys commented Apr 22, 2021

However, I have two concerns regarding this change:

  1. the cmake debian helper seems to cope ok with the existing install variables, I've in fact already packaged libemf2svg (though my packaging is probably not compliant with Debian policies), and the rule file didn't need any tweak, https://github.com/kakwa/amkecpak/blob/master/libemf2svg/debian/rules, the resulting packages are available here (https://mirror.kakwalab.ovh/deb.sid/raw/). However, there are indeed some warnings in the build logs regarding these variables.

Interesting. Maybe things have changed in cmake debian helper since then, as I had to do some minor tweaking. As for warnings, CMake reports unused variables from the command line, so I don't think this is of much importance.

  1. libemf2svg is also packaged by other distributions (Suse and Alpine Linux), and I don't want to risk breaking there packaging
    (https://build.opensuse.org/package/view_file/graphics/libemf2svg/libemf2svg.spec?expand=1). Also, it doesn't seem the cmake macro used by RPM supports the CMAKE_INSTALL* variables: https://src.fedoraproject.org/rpms/cmake/blob/rawhide/f/macros.cmake.

Indeed, my PR as-is would break packaging for these distributions. I wasn't aware that these distributions did not honor CMAKE_INSTALL*. Thus it makes sense to adapt packaging for new distribution to already existing install procedure.

If this change really do make things easier/more compliant, would it be possible for you to rework it so that both *_INSTALL_DIR and CMAKE_INSTALL_*DIR variables work?

This could indeed be done. I will give it a look.

This issue also has been discussed in the past in #22

Thanks for the link.

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.

2 participants