cmake: make symlink in build first in order to support DESTDIR

[gauteh]

#514

[c-alpha]

This installs the symlink in the current directory (likely your build tree). Same goes for line 515.

[gauteh]

c-alpha writes on July 4, 2018 17:00:

c-alpha commented on this pull request. > @@ -509,10 +509,20 @@ install (DIRECTORY ui/icons ) include (InstallSymbolicLink) install_symlink (../../../../astroid/ui/icons/icon_color.png - ${CMAKE_INSTALL_PREFIX}/share/icons/hicolor/512x512/apps/astroid.png + astroid.png This installs the symlink in the current directory (likely your build tree). Same goes for line 515.

Yes - it creates the symlink in the build dir and then installes as a regular built file afterwards. It might be better to create it during build (maybe using a custom target or something)? It seems to work with my simple tests using: DESTDIR=$(pwd)/dist ninja install.

[gauteh]

Gaute Hope writes on July 4, 2018 19:12:

c-alpha writes on July 4, 2018 17:00: > c-alpha commented on this pull request. > >> @@ -509,10 +509,20 @@ install (DIRECTORY ui/icons > ) > include (InstallSymbolicLink) > install_symlink (../../../../astroid/ui/icons/icon_color.png > - ${CMAKE_INSTALL_PREFIX}/share/icons/hicolor/512x512/apps/astroid.png > + astroid.png > > This installs the symlink in the current directory (likely your build tree). Same goes for line 515. > Yes - it creates the symlink in the build dir and then installes as a regular built file afterwards. It might be better to create it during build (maybe using a custom target or something)? It seems to work with my simple tests using: DESTDIR=$(pwd)/dist ninja install.

I changed the approach to just create a copy of the file with the correct name in the build dir and then install that. Removed all the symlink stuff. What do you think about this solution?

[c-alpha]

On 2018-07-05, at 11:31 , Gaute Hope ***@***.***> wrote: [...] I changed the approach to just create a copy of the file with the correct name in the build dir and then install that. Removed all the symlink stuff. What do you think about this solution?

I think we could also install the files directly, and change the name in that process (need to check cmake doc to confirm). We would then have two copies in /usr or /usr/local. Would be nice if we could avoid that, no? Let me first check with @Hello71 what his actual problem is, i.e. what exactly fails with "permission denied", and why. If it should turn out to actually be a permission issue (e.g. /usr/share is rwx------ root.root), then all we can do is add permission checks to the cmake scripts to prompt the user to run the install under sudo.

[gauteh]

c-alpha writes on July 5, 2018 11:48:

On 2018-07-05, at 11:31 , Gaute Hope ***@***.***> wrote: > [...] > I changed the approach to just create a copy of the file with the > correct name in the build dir and then install that. Removed all the > symlink stuff. What do you think about this solution? I think we could also install the files directly, and change the name in that process (need to check cmake doc to confirm).

I have found no CMAKE way to do this.

We would then have two copies in /usr or /usr/local. Would be nice if we could avoid that, no?

Yes, but better to have a clean way to do this.

Let me first check with @Hello71 what his actual problem is, i.e. what exactly fails with "permission denied", and why. If it should turn out to actually be a permission issue (e.g. /usr/share is rwx------ root.root), then all we can do is add permission checks to the cmake scripts to prompt the user to run the install under sudo.

You can check this by doing (not as root!): $ mkdir build $ cd build $ cmake -GNinja .. $ mkdir dest $ DESTDIR=$(pwd)/dest ninja install the final command tries to install the symlinks in the system root, and does not respect the DESTDIR var. This is basically how all packaging systems work. Telling the users to use sudo is not an option, DESTDIR must be supported. The Arch Linux package is therefore also currently broken.

[c-alpha]

On 2018-07-05, at 12:13 , Gaute Hope ***@***.***> wrote: [...] You can check this by doing (not as root!): [...] the final command tries to install the symlinks in the system root, and does not respect the DESTDIR var. This is basically how all packaging systems work. Telling the users to use sudo is not an option, DESTDIR must be supported. The Arch Linux package is therefore also currently broken. [...]

Hm, this sounds like: If CMAKE_INSTALL_DIR has not been set, check whether the DESTDIR environment variable is set, and if so, set CMAKE_INSTALL_DIR to the same value as the DESTDIR environment variable. If neither CMAKE_INSTALL_DIR, nor the DESTDIR environment variable have been set, refuse to install. Maybe the root of the problem is the disconnect between CMAKE_INSTALL_DIR and DESTDIR?

[gauteh]

c-alpha writes on July 5, 2018 12:50:

On 2018-07-05, at 12:13 , Gaute Hope ***@***.***> wrote: > [...] > You can check this by doing (not as root!): > [...] > the final command tries to install the symlinks in the system root, and > does not respect the DESTDIR var. This is basically how all packaging > systems work. Telling the users to use sudo is not an option, DESTDIR > must be supported. The Arch Linux package is therefore also currently broken. > [...] Hm, this sounds like: If CMAKE_INSTALL_DIR has not been set, check whether the DESTDIR environment variable is set, and if so, set CMAKE_INSTALL_DIR to the same value as the DESTDIR environment variable. If neither CMAKE_INSTALL_DIR, nor the DESTDIR environment variable have been set, refuse to install. Maybe the root of the problem is the disconnect between CMAKE_INSTALL_DIR and DESTDIR?

Sounds plausible :)

[c-alpha]

On 2018-07-05, at 12:58 , Gaute Hope ***@***.***> wrote: [...] > Maybe the root of the problem is the disconnect between CMAKE_INSTALL_DIR and DESTDIR? Sounds plausible :)

Hm, given that the Readme.md tells you to do this: $ cmake -H. -Bbuild -GNinja -DCMAKE_INSTALL_PREFIX=/usr/local $ cmake --build build --target install Why would you do $ DESTDIR=$(pwd)/dest ninja install i.e. change the install dir in the very last moment, as you wrote in your previous comment? To change the install dir, you should run cmake again? Or am I missing the point?

[gauteh]

c-alpha writes on July 5, 2018 13:26:

i.e. change the install dir in the very last moment, as you wrote in your previous comment? To change the install dir, you should run cmake again? Or am I missing the point?

Yes :p - excuse the brevity. * CMAKE_INSTALL_PREFIX is not CMAKE_INSTALL_DIR. I don't know about CMAKE_INSTALL_DIR. * CMAKE_INSTALL_PREFIX sets the prefix for the install, like --prefix for autotools. This is what goes into path computations for hard-coded paths in astroid (e.g. resources like icons, theme files, plugins, web-extension). * DESTDIR is like a fake root (AT INSTALL TIME). Together with the point above this make the application only see the CMAKE_INSTALL_PREFIX, and not know about DESTDIR. * The DESTDIR is often a temporary install dir if you want to make a tar / binary package that can be unzipped to root. Other systems use this to install the whole application under a different root. * It is also used for testing the install, e.g. before making a release. * You can change DESTDIR without re-configuring and re-compiling.

[c-alpha]

On 2018-07-05, at 13:45 , Gaute Hope ***@***.***> wrote: [...] * CMAKE_INSTALL_PREFIX is not CMAKE_INSTALL_DIR. I don't know about CMAKE_INSTALL_DIR.

Sorry, my fault. I meant to mean CMAKE_INSTALL_PREFIX, of course.

[...] * DESTDIR is like a fake root (AT INSTALL TIME). Together with the point above this make the application only see the CMAKE_INSTALL_PREFIX, and not know about DESTDIR. * The DESTDIR is often a temporary install dir if you want to make a tar / binary package that can be unzipped to root. Other systems use this to install the whole application under a different root. * It is also used for testing the install, e.g. before making a release. * You can change DESTDIR without re-configuring and re-compiling.

https://cmake.org/cmake/help/latest/envvar/DESTDIR.html#envvar:DESTDIR It seems cmake already generates build scripts that make use of the DESTDIR variable. So I'll have to look into adding DESTDIR support to the symlink macros in our cmake directory. For making a package, we could consider CPack. Havde never used it though...

[gauteh]

c-alpha writes on July 5, 2018 14:06:

It seems cmake already generates build scripts that make use of the DESTDIR variable. So I'll have to look into adding DESTDIR support to the symlink macros in our cmake directory.

Yes, I've used DESTDIR routinely with cmake/astroid. I just didn't notice the permission error. The symlink macro is the only part not supporting the DESTDIR.

For making a package, we could consider CPack. Havde never used it though...

Noticed CPack yesterday actually. Do you think it is worth it? We can probably resurrect the packages from #410 once we release the WebKit2 version.

[c-alpha]

On 2018-07-05, at 14:22 , Gaute Hope ***@***.***> wrote: > [...] > So I'll have to look into adding DESTDIR support to the symlink macros in our cmake directory. Yes, I've used DESTDIR routinely with cmake/astroid. I just didn't notice the permission error. The symlink macro is the only part not supporting the DESTDIR.

Ok, understood (finally). I'll fix the symlink stuff to respect DESTDIR.

> For making a package, we could consider CPack. Havde never used it though... Noticed CPack yesterday actually. Do you think it is worth it? We can probably resurrect the packages from #410 once we release the WebKit2 version. [...]

Um, as I said, no experience with it.

[Hello71]

Let me first check with @Hello71 what his actual problem is, i.e. what exactly fails with "permission denied", and why. If it should turn out to actually be a permission issue (e.g. /usr/share is rwx------ root.root), then all we can do is add permission checks to the cmake scripts to prompt the user to run the install under sudo.

• DESTDIR is like a fake root (AT INSTALL TIME). Together with the point above this make the application only see the CMAKE_INSTALL_PREFIX, and not know about DESTDIR.
• The DESTDIR is often a temporary install dir if you want to make a tar / binary package that can be unzipped to root. Other systems use this to install the whole application under a different root.
• It is also used for testing the install, e.g. before making a release.
• You can change DESTDIR without re-configuring and re-compiling.

In Gentoo, we have a QA feature called "sandbox" which forbids access to the real root during the src_install phase, when make install or ninja install or whatever is called. Packages must not write outside of the DESTDIR during the installation phase in order to pass QA. This is true of every major distribution that I am aware of. Even if it was ignored, however, the way you are doing it now, the symlinks would not be included in the distro package.

[c-alpha]

Thanks @Hello71 for following up, and the clarification. I'm already working on a fix for the cmake scripts.

[gauteh]

Obsoleted by #517.