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.

Sign up for GitHub

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

[build] Fixed support for --libddir under Ubuntu #7094

Closed
wants to merge 4 commits into from
Closed

[build] Fixed support for --libddir under Ubuntu #7094

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
24d35ed
[build] Fixed support for --libddir under Ubuntu
albundy83 Jan 29, 2018
cb3ba8f
Fixed typo
albundy83 Jan 29, 2018
3e926f0
Revert "Fixed typo"
Jan 29, 2018
86bf39e
[build] Fixed missing /
Jan 29, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion config/zfs-build.m4
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -185,7 +185,8 @@ AC_DEFUN([ZFS_AC_RPM], [
RPM_DEFINE_COMMON+=' --define "$(DEBUGINFO_ZFS) 1"'
RPM_DEFINE_COMMON+=' --define "$(ASAN_ZFS) 1"'

RPM_DEFINE_UTIL='--define "_dracutdir $(dracutdir)"'
RPM_DEFINE_UTIL='--define "_libdir $(libdir)"'
RPM_DEFINE_UTIL+=' --define "_dracutdir $(dracutdir)"'
RPM_DEFINE_UTIL+=' --define "_udevdir $(udevdir)"'
RPM_DEFINE_UTIL+=' --define "_udevruledir $(udevruledir)"'
RPM_DEFINE_UTIL+=' --define "_initconfdir $(DEFAULT_INITCONF_DIR)"'
Expand Down
10 changes: 9 additions & 1 deletion rpm/generic/zfs.spec.in
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,13 @@
%global _sbindir /sbin
%global _libdir /%{_lib}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@albundy83 so I think adding a way to override the default libdir is entirely reasonable. But it's not clear to me why this doesn't already default to the right values on Ubuntu. If you take a look at the /usr/lib/rpm/macros file from Ubuntu 16.04 you'll see that both _lib and _libdir are defined correctly for the distribution. Do you know where exactly it's getting the wrong default values from?

%_prefix                /usr
%_exec_prefix           %{_prefix}
%_bindir                %{_exec_prefix}/bin
%_sbindir               %{_exec_prefix}/sbin
%_libexecdir            %{_exec_prefix}/libexec
%_datadir               %{_prefix}/share
%_sysconfdir            /etc
%_sharedstatedir        %{_prefix}/com
%_localstatedir         /var
%_lib                   lib
%_libdir                %{_exec_prefix}/%{_lib}
%_includedir            %{_prefix}/include
%_infodir               %{_datadir}/info
%_mandir                %{_datadir}/man

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this, I did not even know that this file exists :)
I will check on my side.
By the way, when I saw the content of this file, it seems that libdir default is more /usr/lib but according to this for exemple:
https://packages.ubuntu.com/bionic/amd64/libzfs2linux/filelist
libdir is defined in /lib so it seems correct to leave it to /lib no ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Traditionally the packaging defines _prefix to be / so it should default to /lib. Incidentally, if you look down a few lines you'll also see where the %configure macro gets defined.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So, here an update about rpm macros files:

albundy@albundy:~$ rpm --eval '%{_lib}'
lib64
albundy@albundy:~$ rpm --macros=/usr/lib/rpm/macros --eval '%{_lib}'
lib

So, it should work, but when running strace rpm --eval '%{_lib}', we saw (I shorten the output):

...............
open("/usr/lib/rpm/macros", O_RDONLY)   = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=37400, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f4bf7a87000
...............

And later:

...............
open("/usr/lib/rpm/platform/x86_64-linux/macros", O_RDONLY) = 3
fstat(3, {st_mode=S_IFREG|0644, st_size=2121, ...}) = 0
mmap(NULL, 4096, PROT_READ|PROT_WRITE, MAP_PRIVATE|MAP_ANONYMOUS, -1, 0) = 0x7f4bf7a87000
read(3, "# Per-platform rpm configuration"..., 4096) = 2121
read(3, "", 4096)                       = 0
close(3
...............

And the file /usr/lib/rpm/platform/x86_64-linux/macros contain the following entries:

%_lib                   lib64
%_libdir                %{_prefix}/lib64

So it explains why I fallback to /lib64 by default and not /lib

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That makes sense. Then the root cause is that the /usr/lib/rpm/platform/x86_64-linux/macros provided by Debian/Ubuntu is incorrect and specifies the wrong install location for x86_64. Then how about this

diff --git a/config/zfs-build.m4 b/config/zfs-build.m4
index adc99ed..c9a17dc 100644
--- a/config/zfs-build.m4
+++ b/config/zfs-build.m4
@@ -192,6 +192,13 @@ AC_DEFUN([ZFS_AC_RPM], [
        RPM_DEFINE_UTIL+=' $(DEFINE_INITRAMFS)'
        RPM_DEFINE_UTIL+=' $(DEFINE_SYSTEMD)'
 
+       dnl # Override default lib directory on x86_64 Debian/Ubuntu systems.
+       dnl # The provided /usr/lib/rpm/platform/x86_64-linux/macros file
+       dnl # defaults to 'lib64' instead of 'lib/x86_64-linux-gnu'.
+       AS_IF([test "$DEFAULT_PACKAGE" = "deb" -a "$target_cpu" = "x86_64" ], [
+               RPM_DEFINE_UTIL+=' --define "_lib lib/x86_64-linux-gnu"'
+       ])
+
        RPM_DEFINE_KMOD='--define "kernels $(LINUX_VERSION)"'
        RPM_DEFINE_KMOD+=' --define "require_spldir $(SPL)"'
        RPM_DEFINE_KMOD+=' --define "require_splobj $(SPL_OBJ)"'

note: I'm presuming it's set correctly for other arches. This is primarily due to a difference in the way that Debian and Redhat respectively decided to handle the multiarch/multilib issue.

This way it should just do the right thing. One possible concern / advantage is that the updated libraries will now be correctly installed in the same location as the distribution provided ones.

@loli10K can we get your thoughts on this.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is from an armhf Debian8 install:

root@linux:~# grep -H _lib /usr/lib/rpm/platform/*86*/macros
/usr/lib/rpm/platform/i386-linux-gnueabihf/macros:%_libexecdir		%{_prefix}/lib/arm-linux-gnueabihf
/usr/lib/rpm/platform/i386-linux-gnueabihf/macros:%_lib			lib
/usr/lib/rpm/platform/i386-linux-gnueabihf/macros:%_libdir		%{_prefix}/lib
/usr/lib/rpm/platform/i486-linux-gnueabihf/macros:%_libexecdir		%{_prefix}/lib/arm-linux-gnueabihf
/usr/lib/rpm/platform/i486-linux-gnueabihf/macros:%_lib			lib
/usr/lib/rpm/platform/i486-linux-gnueabihf/macros:%_libdir		%{_prefix}/lib
/usr/lib/rpm/platform/i586-linux-gnueabihf/macros:%_libexecdir		%{_prefix}/lib/arm-linux-gnueabihf
/usr/lib/rpm/platform/i586-linux-gnueabihf/macros:%_lib			lib
/usr/lib/rpm/platform/i586-linux-gnueabihf/macros:%_libdir		%{_prefix}/lib
/usr/lib/rpm/platform/i686-linux-gnueabihf/macros:%_libexecdir		%{_prefix}/lib/arm-linux-gnueabihf
/usr/lib/rpm/platform/i686-linux-gnueabihf/macros:%_lib			lib
/usr/lib/rpm/platform/i686-linux-gnueabihf/macros:%_libdir		%{_prefix}/lib
/usr/lib/rpm/platform/x86_64-linux-gnueabihf/macros:%_libexecdir		%{_prefix}/lib/arm-linux-gnueabihf
/usr/lib/rpm/platform/x86_64-linux-gnueabihf/macros:%_lib			lib
/usr/lib/rpm/platform/x86_64-linux-gnueabihf/macros:%_libdir		%{_prefix}/lib

Unless i am mistaking the contents of those platform files they seem wrong and i agree this seems a bug in Debian/Ubuntu-provided macros.

I can probably do more testing to see how the change in zfs-build.m4, which looks good to me, plays with "native" Debian packaging this weekend.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure that defining lib to lib/x86_64-linux-gnu is the correct value, it should be lib no ?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here the way Debian package it:
https://packages.debian.org/sid/amd64/libzfs2linux/filelist
and for Ubuntu:
https://packages.ubuntu.com/bionic/amd64/libzfs2linux/filelist

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So I opted to put them in lib/x86_64-linux-gnu since that's where all the other shared libraries are installed. I'm not sure why they opted to put the zfs libraries under lib, it looks to me like a mistake since they are the only ones installed there.


# Set the default libdir directory based on distribution.
%if %{undefined _libdir}
%if 0%{?fedora} >= 17 || 0%{?rhel} >= 7 || 0%{?centos} >= 7
%global _libdir /%{_lib}
%else
%global _libdir /lib
%endif
Copy link
Contributor

@dinatale2 dinatale2 Jan 29, 2018 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the PR! One comment. I believe you're going to have to add --with-libdir=%{_libdir} to the %configure macro later on in the file.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually, never mind. The %configure macro will pick it up automatically. My mistake.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, exactly, it's automatically added, so no need to add it

%endif

# Set the default udev directory based on distribution.
%if %{undefined _udevdir}
Expand Down