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

4.19 unbuildable on macOS due to Linux-specific extensions #2807

Closed
markdascher opened this issue Dec 11, 2023 · 15 comments
Closed

4.19 unbuildable on macOS due to Linux-specific extensions #2807

markdascher opened this issue Dec 11, 2023 · 15 comments
Labels
bug build Build-system related

Comments

@markdascher
Copy link

This is similar to #2222, but I read the conclusion there as "the RPM team won't bend over backwards to support an OS that isn't POSIX complaint," which makes perfect sense to me. Now, macOS has added the missing POSIX function described in that issue, but 4.20 won't build for different reasons:

The discussion for #2046 even acknowledges that it'll break POSIX compatibility:

Portability sucks, for the developers. If this becomes an actual issue, we'll look into integrating gnulib rather than maintaining our own copy.

It sounds like the Homebrew team has basically given up on trying to build rpm for macOS, so I wanted to open this issue to clarify if that's warranted or not. If we should expect every new version to break POSIX compatibility, then I'd agree that probably makes sense. But I'm not sure if that's the actual intent?

@pmatilai
Copy link
Member

If rpm depends on a non-POSIX extension then it generally is a bug. Such cases just need to be tracked independently, but in brief:

  • rpm-sort was ported from elsewhere and I guess strchrnul() use went unnoticed in review. It needs needs to be fixed not to use that, and in the meanwhile not be built if that's not available.
  • elfdeps needs to be conditionalized on libelf availablility (probably another "lost in cmake translation" victim)
  • the case of GLOB_ONLYDIR is trickier, like noted in the ticket, gnulib could be a solution

@pmatilai
Copy link
Member

pmatilai commented Dec 18, 2023

Oh, looking at the GLOB_ONLYDIR a bit closer: it is and always was an optimization only, so we can simply check for its availability and avoid use if it's not present. Which actually makes it trivial to fix. Addressed in #2812 now too.

@pmatilai pmatilai added bug build Build-system related labels Dec 18, 2023
@markdascher
Copy link
Author

That's great news, thanks! I've never actually tried building this myself, but I gave it a whirl on that branch. Here's some more adjustments that I had to make:

  • Added PRIVATE intl to target_link_libraries for librpm, librpmbuild, librpmio, librpmsign, and a bunch of executables (rpm, rpm2archive, rpmbuild, rpmdb, rpmgraph, rpmkeys, rpmsign, and rpmspec).
    • I don't really understand the difference between PRIVATE and PUBLIC. Maybe putting it somewhere as PUBLIC would've worked better, without having to add it to each thing individually?
    • Setting ENABLE_NLS=OFF makes this unnecessary. But when it's on, I'd think it should link against the necessary libraries automatically.
  • Added PRIVATE iconv to target_link_libraries for librpmbuild.
  • Added PRIVATE popt to target_link_libraries for librpmsign.
  • Added #include <libgen.h> to rpm2archive.c, which seems to be needed for POSIX compatibility. Otherwise it was complaining about the basename call added in Enhance rpm2archive(8) to support cpio format too #2758.
  • Removed PkgConfig::LIBDW and PkgConfig::LIBELF from target_link_libraries for librpmbuild.

Obviously not recommending all of these changes literally–this was just the easiest way for me to map out all of the potential issues remaining.

I tried deciphering #2238 to see which of these might've been lost in the CMake transition, but I figure y'all would be better at figuring that out. I'm also not 100% sure that I'm running cmake exactly as Homebrew would. So if any of this was supposed to work already, then I might've just been doing something strange.

Just wanted to throw this up there in case it's helpful. Thanks again!

@pmatilai
Copy link
Member

Overall that looks just like more cmake transition fallout, uff. I'll look into it.
Thanks for testing + reporting!

PRIVATE vs PUBLIC depends on whether said linkage is private to the library or not, this is relevant for (other) software using these libraries. For example, librpm has PUBLIC linkage to librpmio because in order to use librpm, one must also link to librpmio. However most of rpm's dependencies are of no concern to other software (eg you don't need to link to liblua in order to use librpm), these are PRIVATE linkages.

@pmatilai
Copy link
Member

pmatilai commented Dec 19, 2023

Pushed fixes for most of the above issues to #2812.

The intl case is indeed a bit more complicated. For one, we only check for Intl package, but never actually use it for anything 🤦 and this only works because glibc has this all built it. I'll need to think about that (and preferably find some way to test myself), but at least rpm should be buildable now, with ENABLE_NLS=OFF.

@pmatilai
Copy link
Member

pmatilai commented Dec 19, 2023

Does the following, on top of #2812, make it buildable with ENABLE_NLS? (not sure this is the best way to do it but ...)

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 24dfcbb95..596f353b4 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -220,6 +220,9 @@ endif()
 
 if (ENABLE_NLS)
        find_package(Intl REQUIRED)
+       include_directories(${Intl_INCLUDE_DIRS})
+       link_directories(${Intl_LIBRARY_DIRS})
+       link_libraries(${Intl_LIBRARIES})
 endif()
 
 if (ENABLE_SQLITE)

@markdascher
Copy link
Author

markdascher commented Dec 19, 2023

Almost! 🙂

I didn't realize that cmake was supposed to handle this automatically, and until now I'd been setting CFLAGS=-I/usr/local/include (basically hoping that homebrew did something similar). The include_directories(${Intl_INCLUDE_DIRS}) makes that unnecessary because Intl_INCLUDE_DIRS is /usr/local/include, but that also makes my local rpm headers fight with the headers in the build. So I tried taking this advice and rearranging things:

We don't model any ordering dependencies among include directories so you may need to order your [target_]include_directories calls accordingly.

So if I move include_directories(${Intl_INCLUDE_DIRS}) down below include_directories(${CMAKE_SOURCE_DIR}/include), it all works.

Also the link_directories isn't needed, since Intl_LIBRARIES is an absolute path.

@pmatilai
Copy link
Member

Thanks for testing!

That's also a fine reminder of how global includes and the like get problematic real fast, so I ended up pushing a target-based version to the PR instead. So unless I screwed something up, the PR should be compilable on a whole bunch of platforms where >= 4.19.0 previously was not.

@markdascher
Copy link
Author

[  1%] Building C object misc/CMakeFiles/libmisc.dir/fts.c.o
In file included from misc/fts.c:76:
misc/system.h:87:11: fatal error: 'libintl.h' file not found

Added to misc/CMakeLists.txt:
target_include_directories(libmisc PRIVATE ${Intl_INCLUDE_DIRS})

[  2%] Building C object rpmio/rpmpgp_legacy/CMakeFiles/rpmpgp_legacy.dir/rpmpgp_internal.c.o
In file included from rpmio/rpmpgp_legacy/rpmpgp_internal.c:6:
misc/system.h:87:11: fatal error: 'libintl.h' file not found

Added to rpmio/rpmpgp_legacy/CMakeLists.txt:
target_include_directories(rpmpgp_legacy PRIVATE ${Intl_INCLUDE_DIRS})

[ 88%] Building C object python/CMakeFiles/_rpm.dir/rpmmodule.c.o
In file included from python/rpmmodule.c:26:
In file included from python/spec-py.h:4:
In file included from include/rpm/rpmbuild.h:9:
include/rpm/rpmcli.h:10:10: fatal error: 'popt.h' file not found

Added to python/CMakeLists.txt:
target_include_directories(_rpm PRIVATE ${POPT_INCLUDE_DIRS})

And then everything worked.

I think that last one got hidden when we tried globally including Intl_INCLUDE_DIRS, just because it's /usr/local/include which includes a copy of popt.h too.

@pmatilai
Copy link
Member

pmatilai commented Jan 4, 2024

include/rpm/rpmcli.h:10:10: fatal error: 'popt.h' file not found

Added to python/CMakeLists.txt:
target_include_directories(_rpm PRIVATE ${POPT_INCLUDE_DIRS})

Actually popt needs to be a public include directory for rpm because of that rpmcli.h include, I didn't realize/remember we had such a thing...

But okay, we're getting close now.

@pmatilai
Copy link
Member

pmatilai commented Jan 4, 2024

Hmm, actually there shouldn't be any need for rpmbuild.h to include rpmcli.h either.

@pmatilai
Copy link
Member

pmatilai commented Jan 4, 2024

Pushed another update to the PR, hopefully that covers these final bits too.

@markdascher markdascher changed the title 4.20 unbuildable on macOS due to Linux-specific extensions 4.19 unbuildable on macOS due to Linux-specific extensions Jan 4, 2024
@markdascher
Copy link
Author

markdascher commented Jan 4, 2024

Bingo! It works now:

mkdir _build
cd _build

PKG_CONFIG_PATH=/usr/local/opt/libarchive/lib/pkgconfig cmake -DENABLE_NLS=ON -DENABLE_PLUGINS=OFF -DWITH_AUDIT=OFF -DWITH_INTERNAL_OPENPGP=ON -DWITH_OPENSSL=ON -DWITH_SELINUX=OFF -DENABLE_TESTSUITE=OFF -DWITH_ACL=OFF -DWITH_CAP=OFF ..
make
# …
# [100%] Built target _rpm

./tools/rpm --version
# RPM version 4.19.90

Those cmake options are basically just the args listed in Homebrew's rpm.rb, plus -DWITH_ACL=OFF -DWITH_CAP=OFF since I'm guessing macOS doesn't support those features, and they were disabled by default in previous versions.

So I believe that PR solves this issue completely. Happy new year!

@pmatilai
Copy link
Member

pmatilai commented Jan 8, 2024

Excellent! 🥳

And again, thank you for reporting, suggesting fixes and testing. This is how it works 👍
It's also worth noting that absolutely nothing at all here was specific to macOS, just more fallout from the cmake switch that has gotten masked by one thing or another on (some) Linux distros.

@pmatilai
Copy link
Member

pmatilai commented Jan 8, 2024

As per above report, fixed by #2812

There's nothing controversial in the PR so those who need non-Linux builds before 4.19.2 gets released, just apply the PR as a whole. At least cmake makes applying buildsys related patches sane.

@pmatilai pmatilai closed this as completed Jan 8, 2024
lionelfleury pushed a commit to lionelfleury/homebrew-core that referenced this issue Feb 21, 2024
RPM 4.19.1.1 can be built on MacOS again. See rpm-software-management/rpm#2807 for details.
lionelfleury pushed a commit to lionelfleury/homebrew-core that referenced this issue Feb 21, 2024
RPM 4.19.1.1 can be built on MacOS again. See rpm-software-management/rpm#2807 for details.
lionelfleury pushed a commit to lionelfleury/homebrew-core that referenced this issue Feb 21, 2024
RPM 4.19.1.1 can be built on MacOS again. See rpm-software-management/rpm#2807 for details.
lionelfleury pushed a commit to lionelfleury/homebrew-core that referenced this issue Feb 21, 2024
RPM 4.19.1.1 can be built on MacOS again. See rpm-software-management/rpm#2807 for details.
lionelfleury pushed a commit to lionelfleury/homebrew-core that referenced this issue Feb 21, 2024
RPM 4.19.1.1 can be built on MacOS again. See rpm-software-management/rpm#2807 for details.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug build Build-system related
Projects
None yet
Development

No branches or pull requests

2 participants