onigumura: fix library installation#29397
Conversation
|
I can confirm this fixes php8-mod-mbstring which depends on oniguruma. |
| define Package/oniguruma/install | ||
| $(INSTALL_DIR) $(1)/usr/lib | ||
| $(CP) $(PKG_INSTALL_DIR)/usr/lib/libonig.so.$(ABI_VERSION) $(1)/usr/lib/ | ||
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/lib/libonig.so.$(ABI_VERSION) $(1)/usr/lib/ |
There was a problem hiding this comment.
Oh, I figured out what went wrong. I pushed the commit almost completely without the commit description. It was heavily inspired by openwrt/openwrt@38b22b1 and #9233
You are right, my change is not sufficient and breaks the library, but here is what is going on. To understand this issue, we need to look at two use-cases. One with ABI_VERSION, which I broke, and the other one without ABI_VERSION, which was fixed.
With ABI versioning:
Before:
tar -Oxzf bin/packages/arm_cortex-a9_vfpv3-d16/packages/oniguruma_6.9.9-r1_arm_cortex-a9_vfpv3-d16.ipk ./data.tar.gz | tar -tzvf -
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/lib/
-rwxr-xr-x 0 0 0 526493 Feb 5 2024 ./usr/lib/libonig.so.5
Current master - basically broken:
tar -Oxzf bin/packages/arm_cortex-a9_vfpv3-d16/packages/oniguruma_6.9.9-r1_arm_cortex-a9_vfpv3-d16.ipk ./data.tar.gz | tar -tzvf -
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/lib/
lrwxrwxrwx 0 0 0 0 Feb 5 2024 ./usr/lib/libonig.so.5 -> libonig.so.5.4.0
After applying this fix (just adding *):
| $(INSTALL_BIN) $(PKG_INSTALL_DIR)/usr/lib/libonig.so.$(ABI_VERSION) $(1)/usr/lib/ | |
| $(CP) $(PKG_INSTALL_DIR)/usr/lib/libonig.so.$(ABI_VERSION)* $(1)/usr/lib/ |
It looks like this:
tar -Oxzf bin/packages/arm_cortex-a9_vfpv3-d16/packages/oniguruma_6.9.9-r1_arm_cortex-a9_vfpv3-d16.ipk ./data.tar.gz | tar -tzvf -
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/
drwxr-xr-x 0 0 0 0 Feb 5 2024 ./usr/lib/
lrwxrwxrwx 0 0 0 0 Feb 5 2024 ./usr/lib/libonig.so.5 -> libonig.so.5.4.0
-rwxr-xr-x 0 0 0 526493 Feb 5 2024 ./usr/lib/libonig.so.5.4.0
Without ABI versioning:
- libre2 package
Before:
tar -Oxzf bin/packages/arm_cortex-a9_vfpv3-d16/packages/re2_2023.02.01\~b025c6a3-r1_arm_cortex-a9_vfpv3-d16.ipk ./data.tar.gz | tar -tzvf -
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./usr/
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./usr/lib/
-rw-r--r-- 0 0 0 331875 Aug 18 2024 ./usr/lib/libre2.so
-rw-r--r-- 0 0 0 331875 Aug 18 2024 ./usr/lib/libre2.so.10
-rw-r--r-- 0 0 0 331875 Aug 18 2024 ./usr/lib/libre2.so.10.0.0
After applying my commit:
tar -Oxzf bin/packages/arm_cortex-a9_vfpv3-d16/packages/re2_2023.02.01\~b025c6a3-r1_arm_cortex-a9_vfpv3-d16.ipk ./data.tar.gz | tar -tzvf -
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./usr/
drwxr-xr-x 0 0 0 0 Aug 18 2024 ./usr/lib/
lrwxrwxrwx 0 0 0 0 Aug 18 2024 ./usr/lib/libre2.so -> libre2.so.10
lrwxrwxrwx 0 0 0 0 Aug 18 2024 ./usr/lib/libre2.so.10 -> libre2.so.10.0.0
-rwxr-xr-x 0 0 0 331875 Aug 18 2024 ./usr/lib/libre2.so.10.0.0
To sum it up:
- With ABI_VERSION, there was no symlink. Was this intentional?
- Without ABI_VERSION, we were shipping three (!) files in case of libre2, which makes it huge and no symlinks at all.
I am not entirely sure how this pattern originated in the packages feed. If I look at the main openwrt repository, ABI_VERSION is not typically used this way in the Package/foo/install definitions. For instance, I checked the jansson package in the main repo, and it correctly ships the symlink when using ABI.
TL,DR: If I am not mistaken, even packages with ABI_VERSION should preserve the correct symlink structure pointing to the actual shared object. Can we agree on this one? If yes, I will prepare a new PR to fix other packages, which I broke. 😢
There was a problem hiding this comment.
I can't say for other packages, but my installing of libonig.so.5 only is intentional. That's the only file the OS needs to load the library. The unversioned libonig.so would be used to link the binary when building a binary. It could arguably be used if one were to use an openwrt host for building binaries. However, our package system does not handle multiple packages owning the symlink well, so I never tried to head in that direction.
Why do we need the libonig.so.5.4.0 file? To me it is a needless symlink that will mostly clutter /usr/lib.
TLDR: I would leave the package as I'm doing here.
There was a problem hiding this comment.
Thanks, and I see your point about saving an inode and keeping the package layout minimal. My concern is mainly consistency across OpenWrt and easier debugging in practice.
When I look at /usr/lib on a running router, the symlinked layout is what I see for the large majority of shared libraries, including core OpenWrt ones. From my point of view, the bigger problem is having several different packaging styles for essentially the same kind of library, because that makes the feed harder to maintain and makes it less obvious which approach is considered correct.
In my view, it is more useful to know the exact library version that is present, not just the major version. Keeping only the major version visible does not seem like a good trade-off to me, especially when the symlink itself uses very little space. With ls -l, you can immediately see which exact library version is present. You can also inspect package contents through apk info -L oniguruma or opkg files oniguruma, but that still does not make the full version visible in the same straightforward way if only the major-version name is kept. It becomes even more confusing when the package version does not directly match the library version, because then it is no longer obvious which exact library build is actually installed unless the full version is visible in /usr/lib.
What I found is that The TLDP Program Library HOWTO explicitly describes the usual layout as SONAME -> Real Name, stating that a fully qualified soname is a symbolic link to the shared library’s real name, and that ldconfig creates sonames as symbolic links to the real names. The GNU Libtool manual’s Release numbers page also shows the same general idea of using a symlinked library name that points to the actual versioned file, although it is less explicit about SONAME terminology there. For that reason, I do not think omitting the library’s full release number is a good idea.
That being said, you are the maintainer of this package, so I will respect your decision here. I would just prefer that we keep one consistent shared-library layout across the feed, because mixing both approaches makes maintenance and debugging harder than necessary, and then we end up having to debate what is actually correct, why it should be done that way, and what the expected layout is.
There was a problem hiding this comment.
And it looks like CI/CD agrees with me. :)
|
Completely off-topic, but I just noticed that the oniguruma repository is read-only (see https://github.com/kkos/oniguruma/) and that development has basically ended based on this commit - kkos/oniguruma@f95747b Shouldn't we consider removing this package? |
Moved here? https://github.com/youkidearitai/oniguruma |
php8-mod-mbstring, at least, depends on it. Might want to file a bug against PHP8 and get that straightened away before doing that. |
|
As for removing the package, unless there's a stronger development commitment, it will eventually happen. We should wait for dependent packages to adapt themselves, assuming there's no security issue. I wouldn't count on it, considering the rising rate at which vulnerabilities are being found recently. |
I took a look, but it only has 3 more commits, and just one of them is actually relevant:
There's already an issue open for it, along with a pull request: php/php-src#18467 and php/php-src#21490 |
|
Looks like the PR has been merged in PHP8. So we could (safely) drop Oniguruma once that winds its way through the process and packaging. |
Commit 537c2a6 ("treewide: avoid deref symlinks when installing .so") intended to avoid duplicating .so* files, but this package actually relies on install dereferencing the file that matches the SONAME version, to avoid installing unnecessary symlinks. Fixes: openwrt#29387 Fixes: 537c2a6 ("treewide: avoid deref symlinks when installing .so") Signed-off-by: Eneas U de Queiroz <cotequeiroz@gmail.com>
|
I'll merge this as is. @BKPepe, feel free to change it to include the extra symlink if you wish. |
4 participants
Commit 537c2a6 ("treewide: avoid deref symlinks when installing .so") intended to avoid duplicating .so* files, but this package actually relies on install dereferencing the file that matches the SONAME version, to avoid installing unnecessary symlinks.
Fixes: #29387
Fixes: 537c2a6 ("treewide: avoid deref symlinks when installing .so")
Ping @BKPepe