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

fish assumes CMAKE_INSTALL_DATADIR must be relative #8150

Closed
jpalus opened this issue Jul 19, 2021 · 5 comments · Fixed by #8151
Closed

fish assumes CMAKE_INSTALL_DATADIR must be relative #8150

jpalus opened this issue Jul 19, 2021 · 5 comments · Fixed by #8151
Labels
Milestone

Comments

@jpalus
Copy link
Contributor

jpalus commented Jul 19, 2021

CMAKE_INSTALL_DATADIR can be both relative, in which case CMake will add CMAKE_INSTALL_PREFIX or absolute when prefix is not added. fish assumes only relative path can be passed and if absolute path is used pkgconfig file is broken with prefix added:

set(rel_datadir ${CMAKE_INSTALL_DATADIR})

fish-shell/fish.pc.in

Lines 1 to 2 in b0dc72e

prefix=@prefix@
datadir=${prefix}/@rel_datadir@

@faho
Copy link
Member

faho commented Jul 19, 2021

And you noticed this because?

Having these technically wrong things is always nice, but I'd prefer to have a usecase. What were you trying to do?

What would your preferred fix be? Canonicalizing the path before using it? Erroring out if an absolute path is passed?

@faho faho added the packaging label Jul 19, 2021
@faho faho added this to the fish-future milestone Jul 19, 2021
@jpalus
Copy link
Contributor Author

jpalus commented Jul 19, 2021

In our RPM based Linux distribution %cmake macro is used as a standard way to configure CMake based projects in a consistent way. That includes common (absolute) paths like %{_bindir} or %{_datadir} which end up in CMAKE_INSTALL_BINDIR and CMAKE_INSTALL_DATADIR respectively. After switching from autotools based fish version to one using cmake exclusively we've observed this regression. Note that for the time being it is patched on our level with:

--- fish-3.3.1/cmake/Install.cmake.orig 2021-07-06 16:45:37.000000000 +0200
+++ fish-3.3.1/cmake/Install.cmake      2021-07-19 20:04:46.496541738 +0200
@@ -14,7 +14,11 @@
 set(sysconfdir ${CMAKE_INSTALL_SYSCONFDIR})
 set(mandir ${CMAKE_INSTALL_MANDIR})
 
-set(rel_datadir ${CMAKE_INSTALL_DATADIR})
+if(IS_ABSOLUTE ${CMAKE_INSTALL_DATADIR})
+  file(RELATIVE_PATH rel_datadir ${CMAKE_INSTALL_PREFIX} ${CMAKE_INSTALL_DATADIR})
+else()
+  set(rel_datadir ${CMAKE_INSTALL_DATADIR})
+endif()
 set(datadir ${CMAKE_INSTALL_FULL_DATADIR})
 
 set(docdir ${CMAKE_INSTALL_DOCDIR})

@faho
Copy link
Member

faho commented Jul 19, 2021

Okay, so why do we not encounter the same issue in our spec file (I'm assuming, having never actually built an rpm)? Is it just because we've never tried an absolute path?

Why do y'all pass absolute paths here? I've never really seen anything other than paths relative to the prefix.

Note that for the time being it is patched on our level with:

Ah, I'm assuming that computes the relative path from the absolute one, so it can then later prepend the PREFIX. And you'd get a path like ../../foo if DATADIR was outside of PREFIX?

That should work. If you want to open a PR that'd be great. (in general I'd love it if distro maintainers tried to upstream their stuff earlier - I've had to do that myself for some other things)

@zanchey should probably weigh in on this.

jpalus added a commit to jpalus/fish-shell that referenced this issue Jul 19, 2021
Cmake accepts both absolute and relative paths in CMAKE_INSTALL_DATADIR.
For the latter case CMAKE_INSTALL_PREFIX is being prepended
automatically. %{rel_datadir} is derived from CMAKE_INSTALL_DATADIR
which was assumed to be relative and otherwise causes issues in a .pc
file where prefix is being prepended unconditionally.

Make sure %{rel_datadir} is relative by calculating RELATIVE_PATH from
CMAKE_INSTALL_PREFIX to CMAKE_INSTALL_FULL_DATADIR which is known to be
absolute.

Fixes fish-shell#8150
@jpalus
Copy link
Contributor Author

jpalus commented Jul 19, 2021

Okay, so why do we not encounter the same issue in our spec file (I'm assuming, having never actually built an rpm)? Is it just because we've never tried an absolute path?

Why do y'all pass absolute paths here? I've never really seen anything other than paths relative to the prefix.

I assume your spec file targets RH/Fedora where %cmake macro looks differently and does not specify CMAKE_INSTALL_DATADIR explicitly - only CMAKE_INSTALL_PREFIX is defined. That means CMAKE_INSTALL_DATADIR defaults to relative share. What that also means is that RH/Fedora do not respect user customization of %{_datadir} macro (or any other similar macro for bin/doc/include/etc) like in all other specs ie for software based on autotools. That's the inconsistency our distribution wants to avoid.

Ah, I'm assuming that computes the relative path from the absolute one, so it can then later prepend the PREFIX. And you'd get a path like ../../foo if DATADIR was outside of PREFIX?

Correct.

That should work. If you want to open a PR that'd be great. (in general I'd love it if distro maintainers tried to upstream their stuff earlier - I've had to do that myself for some other things)

#8151 opened with equivalent but more concise fix.

@zanchey
Copy link
Member

zanchey commented Jul 20, 2021

Yes, I think it would be reasonable to take a patch. I'm not sure why anyone wants to do this but I'm sure there are reasons!

faho pushed a commit that referenced this issue Jul 20, 2021
Cmake accepts both absolute and relative paths in CMAKE_INSTALL_DATADIR.
For the latter case CMAKE_INSTALL_PREFIX is being prepended
automatically. %{rel_datadir} is derived from CMAKE_INSTALL_DATADIR
which was assumed to be relative and otherwise causes issues in a .pc
file where prefix is being prepended unconditionally.

Make sure %{rel_datadir} is relative by calculating RELATIVE_PATH from
CMAKE_INSTALL_PREFIX to CMAKE_INSTALL_FULL_DATADIR which is known to be
absolute.

Fixes #8150
@zanchey zanchey added cmake and removed packaging labels Jul 20, 2021
@zanchey zanchey modified the milestones: fish-future, fish 3.4.0 Jul 20, 2021
thunder-coding pushed a commit to thunder-coding/fish-shell that referenced this issue Jul 28, 2021
Cmake accepts both absolute and relative paths in CMAKE_INSTALL_DATADIR.
For the latter case CMAKE_INSTALL_PREFIX is being prepended
automatically. %{rel_datadir} is derived from CMAKE_INSTALL_DATADIR
which was assumed to be relative and otherwise causes issues in a .pc
file where prefix is being prepended unconditionally.

Make sure %{rel_datadir} is relative by calculating RELATIVE_PATH from
CMAKE_INSTALL_PREFIX to CMAKE_INSTALL_FULL_DATADIR which is known to be
absolute.

Fixes fish-shell#8150
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 25, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants