search

letsfindaway / OpenBoard

I'm using this fork to contribute features and fixes to the upstream project. In order to create good pull requests, I'm rebasing my feature branches, squashing and reordering commits, etc. If you fork this repository be aware that my development branches may rewrite history without prior notice.
http://openboard.ch/
GNU General Public License v3.0
9 stars 0 forks source link

[chore-cmake-community-builds] Unnecessary DESTDIR variable used for paths in UBPlatformUtils for Linux #157

Closed Vekhir closed 1 year ago

Vekhir commented 1 year ago

@letsfindaway Please remove all mentions of DESTDIR. Arch uses it for packaging purposes, so the program does not need to be aware of it. I don't think it's necessary to specifically hint at the existence of this variable either - those who need it will find it.

The relevant section to be changed is here: https://github.com/letsfindaway/OpenBoard/blob/chore-cmake-community-builds/src/frameworks/UBPlatformUtils_linux.cpp#L59-L87 And also this line: https://github.com/letsfindaway/OpenBoard/blob/chore-cmake-community-builds/CMakeLists.txt#L25

letsfindaway commented 1 year ago

Hi,

I added this mechanism to ease testing. So let me first explain my intention and then look for ways to solve this.

During development, I open the CMakeLists.txt file as project file in QtCreator. When I want to test my work, then the platform utils must somehow point to directories which contain the respective files. Without that code they would however return something like /usr/share/openboard, but at this location is either nothing (when I have not installed OpenBoard) or files from another installation, which might not be what I'm testing.

So I need a way to reference the correct files from the project under test during testing. What I do is to add an install step to the builds in QtCreator. I set the DESTDIR variable to an arbitrary directory which will then contain the full package files. If I now also use DESTDIR during execution of my test builds, then I get exactly the expected files at these paths.

I agree that this is only useful for testing and never in a deployed package, so my proposal would be to wrap this into

#infdef NDEBUG
#endif

so that this code is not compiled into release builds! Would this be ok?

letsfindaway commented 1 year ago

And why would you remove the hint from the comments in the CMakeLists.txt? I would never recommend to run cmake --install without DESTDIR. I can imagine many semi-experienced users who then stumble about the "missing privileges" during the install step and are then trying to execute this command with sudo. They would better be off with this hint and eventually even use cpack to create a package.

Vekhir commented 1 year ago

Hi,

I added this mechanism to ease testing. So let me first explain my intention and the look for ways to solve this.

During development, I open the CMakeLists.txt file as project file in QtCreator. When I want to test my work, then the platform utils must somehow point to directories which contain the respective files. Without that code they would however return something like /usr/share/openboard, but at this location is either nothing (when I have not installed OpenBoard) or files from another installation, which might not be what I'm testing.

So I need a way to reference the correct files from the project under test during testing. What I do is to add an install step to the builds in QtCreator. I set the DESTDIR variable to an arbitrary directory which will then contain the full package files. If I now also use DESTDIR during execution of my test builds, then I get exactly the expected files at these paths.

To me it seems that might be better served by setting CMAKE_INSTALL_PREFIX and CMAKE_INSTALL_SYSCONFDIR. Is there a reason that's not possible? (I.e. set CMAKE_INSTALL_PREFIX to $DESTDIR/usr and CMAKE_INSTALL_SYSCONFDIR to $DESTDIR/etc

I agree that this is only useful for testing and never in a deployed package, so my proposal would be to wrap this into

#infdef NDEBUG
#endif

so that this code is not compiled into release builds! Would this be ok?

At the moment I'm not convinced it's necessary, but might be worth pursuing as a last resort.

And why would you remove the hint from the comments in the CMakeLists.txt? I would never recommend to run cmake --install without DESTDIR. I can imagine many semi-experienced users who then stumble about the "missing privileges" during the install step and are then trying to execute this command with sudo. They would better be off with this hint and eventually even use cpack to create a package.

And what value do you suggest they use? Any value apart from "" (which is default and runs into permission issues for obvious reasons) would not be FHS-compliant. We are installing a package after all and nobody expects apt to run without sudo. Of course using the distros packaging utilities should be the first thought. I guess it is reasonable to first "install" it to a local directory before copying them (with sudo) to their final installation. But this isn't apparent from your hint so who knows what that 'semi-experienced' user is gonna do, and I don't think it's our job to explain how cmake works anyway (in a CMakeLists.txt of all places). Perhaps a link to a more extensive guide is better suited for that? Or put it in the README or the Wiki once merged. I've have just never before seen anyone suggest using DESTDIR as a step before cmake --install or make install when installing locally.

letsfindaway commented 1 year ago

If I now also use DESTDIR during execution of my test builds, then I get exactly the expected files at these paths.

To me it seems that might be better served by setting CMAKE_INSTALL_PREFIX and CMAKE_INSTALL_SYSCONFDIR. Is there a reason that's not possible? (I.e. set CMAKE_INSTALL_PREFIX to $DESTDIR/usr and CMAKE_INSTALL_SYSCONFDIR to $DESTDIR/etc

This does not give correct results if I want to use a relative directory. E.g. setting

creates the etc files in ./install/usr/install/etc, because cmake install with a relative DESTINATION will prepend the CMAKE_INSTALL_PREFIX. So I would have to configure absolute, long paths, something like /home/user/Documents/projects/github/openboard/devel/install/usr, which is annoying. I have not found any workaround - except DESTDIR.

I agree that this is only useful for testing and never in a deployed package, so my proposal would be to wrap this into

#infdef NDEBUG
#endif

so that this code is not compiled into release builds! Would this be ok?

At the moment I'm not convinced it's necessary, but might be worth pursuing as a last resort.

And why would you remove the hint from the comments in the CMakeLists.txt? I would never recommend to run cmake --install without DESTDIR. I can imagine many semi-experienced users who then stumble about the "missing privileges" during the install step and are then trying to execute this command with sudo. They would better be off with this hint and eventually even use cpack to create a package.

And what value do you suggest they use? Any value apart from "" (which is default and runs into permission issues for obvious reasons) would not be FHS-compliant.

Every DESTDIR located in the user's home is FHS compliant, because the content of this directory for a not-empty DESTDIR is always considered to be user data and not part of the system.

We are installing a package after all and nobody expects apt to run without sudo.

No, we're testing a program here. That might be the missing point. I was installing packages from source using make install about 20 years ago, but now never think about that. When I compile a program for local installation then I would either create a package and install that using the package manager or not install it at all, but keep it in some local directory in my home. But I see that many sources on the Internet recommend sudo make install in the hope that it goes to /usr/local. Might work but I personally would never do that.

Of course using the distros packaging utilities should be the first thought. I guess it is reasonable to first "install" it to a local directory before copying them (with sudo) to their final installation. But this isn't apparent from your hint so who knows what that 'semi-experienced' user is gonna do, and I don't think it's our job to explain how cmake works anyway (in a CMakeLists.txt of all places).

This is a valid argument.

Perhaps a link to a more extensive guide is better suited for that? Or put it in the README or the Wiki once merged.

I also like the idea of a wiki page. It already contains a Howto for qmake builds on the various platforms. We could accompany this by a guide how to use cmake for development and local testing. Again even a wiki is not the place to tell how packaging on various distros work.

I've have just never before seen anyone suggest using DESTDIR as a step before cmake --install or make install when installing locally.

Again, I don't install locally. I just want to provide a runnable directory structure for testing which mimics the final package layout.

Vekhir commented 1 year ago

If I now also use DESTDIR during execution of my test builds, then I get exactly the expected files at these paths.

To me it seems that might be better served by setting CMAKE_INSTALL_PREFIX and CMAKE_INSTALL_SYSCONFDIR. Is there a reason that's not possible? (I.e. set CMAKE_INSTALL_PREFIX to $DESTDIR/usr and CMAKE_INSTALL_SYSCONFDIR to $DESTDIR/etc

This does not give correct results if I want to use a relative directory. E.g. setting

  • CMAKE_INSTALL_PREFIX:PATH=./install/usr and
  • CMAKE_INSTALL_SYSCONFDIR:PATH=./install/etc

creates the etc files in ./install/usr/install/etc, because cmake install with a relative DESTINATION will prepend the CMAKE_INSTALL_PREFIX. So I would have to configure absolute, long paths, something like /home/user/Documents/projects/github/openboard/devel/install/usr, which is annoying. I have not found any workaround - except DESTDIR.

Yeah, I meant absolute paths. Isn't this something one has to do round about once in a config somewhere? I understand how it can be annoying, but a setting that's "set and forget" doesn't strike me as worthy of a workaround. The thing is that DESTDIR is only implemented for staging, like what Arch does, and it's not intended that the program be run from there, for testing or otherwise. GNU Make about DESTDIR:

Prepending the variable DESTDIR to each target in this way provides for staged installs,
where the installed files are not placed directly into their expected location but are instead
copied into a temporary location (DESTDIR).

I was installing packages from source using make install about 20 years ago, but now never think about that. When I compile a program for local installation then I would either create a package and install that using the package manager or not install it at all, but keep it in some local directory in my home. But I see that many sources on the Internet recommend sudo make install in the hope that it goes to /usr/local. Might work but I personally would never do that.

Same here, I had a similar journey. The educational issue here is, I guess, that many people (on Linux) can't or don't know how to create a package for their distro. So they do what we developers tell them, but we can only tell them sudo make install, because that's the only thing all distros have in common. In fact, a PKGBUILD is just that - almost all packages can be created just by copying what the developers say (and looking at the Arch Wiki, of course). I should say though that I have no experience with either .deb nor .rpm packages.

letsfindaway commented 1 year ago

The thing is that DESTDIR is only implemented for staging, like what Arch does, and it's not intended that the program be run from there, for testing or otherwise.

Would you be happier if the environment variable was called OPENBOARD_BASE or so? That's something I could easily live with.

Same here, I had a similar journey. The educational issue here is, I guess, that many people (on Linux) can't or don't know how to create a package for their distro.

With cmake, they only have to invoke cpack -G DEB to create a Debian package or cpack -G RPM for an .rpm, at least good enough for local use. There is indeed no need to invoke cmake --install, so I agree, I can remove that line from the instructions.

Vekhir commented 1 year ago

The thing is that DESTDIR is only implemented for staging, like what Arch does, and it's not intended that the program be run from there, for testing or otherwise.

Would you be happier if the environment variable was called OPENBOARD_BASE or so? That's something I could easily live with.

It's a code smell either way. Though naming it something like OPENBOARD_TESTING_BASEDIR and putting that behind an #ifdef DEBUG goes a long way to make it not smell as badly. If you are fine with that, I'd consider this issue solved.