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

WIP: Port libarchive to CMake #2602

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

ongun-kanat
Copy link

@ongun-kanat ongun-kanat commented Aug 13, 2021

This is my first contribution to the repository so I may need some changes. This is a WIP PR. I want to make sure the build works and meets the expectations before merging it.

Since the CMakeLists provided the option for dynamically linking bsdtar and bsdcpio with libarchive, I enabled that setting. Therefore bsdtar and bsdcpio now depend on libarchive which provides all the other dependencies.

Unfortunately I cannot build autotools version of libarchive. Some autoconf rules seem to be missing in my default MSYS2 installation with base-devel group of packages (missing makedepends() ?). Therefore, I cannot make a 1-to-1 comparison.

I plan to add a check() function to PKGBUILD in order to run make test. Currently a few of them fails/segfaults. The fails seem to be actual programming errors in libarchive and they may need to be patched.


Long logfile of failed tests
LastTest.log

This port also dynamically links bsdtar and bsdcpio to libarchive.
Therefore bsdtar and bsdcpio now depend on libarchive.
@lazka
Copy link
Member

lazka commented Aug 13, 2021

Therefore, I cannot make a 1-to-1 comparison.

The CI output shows a filesystem level diff at least.

libarchive/PKGBUILD Outdated Show resolved Hide resolved

# CNG breaks pacman on Vista due to BCryptDeriveKeyPBKDF2
cmake \
-G"Unix Makefiles" \
Copy link
Contributor

Choose a reason for hiding this comment

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

Have you tried using Ninja?

Copy link
Author

Choose a reason for hiding this comment

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

I didn't since I wasn't sure DESTDIR worked with Ninja. Apparently it works and now PKGBUILD uses Ninja. However this issue with Ninja is annoying: ninja-build/ninja#1482

Copy link
Member

Choose a reason for hiding this comment

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

Yes it is. CMAKE_BUILD_PARALLEL_LEVEL can be used with cmake --build (instead of calling ninja directly)

Copy link
Author

Choose a reason for hiding this comment

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

Hmm I should amend the commit with that. Thanks for the info @jeremyd2019

@ongun-kanat
Copy link
Author

Therefore, I cannot make a 1-to-1 comparison.

The CI output shows a filesystem level diff at least.

libarchive seems to use a unique way to give "sonames" to their libraries: https://github.com/libarchive/libarchive/blob/3.5.1/CMakeLists.txt#L74 So it changes file name to *-18.dll

@jeremyd2019
Copy link
Member

jeremyd2019 commented Aug 16, 2021

If soname changes, all dependencies need to be rebuilt. Luckily there aren't too many: https://packages.msys2.org/package/libarchive?repo=msys&variant=x86_64 (see "Required by" section)

@ongun-kanat
Copy link
Author

If soname changes, all dependencies need to be rebuilt. Luckily there aren't too many: https://packages.msys2.org/package/libarchive?repo=msys&variant=x86_64 (see "Required by" section)

I know :) I haven't investigated libarchive source code history to have enough confidence in ABI stability of their interfaces. If they want to increment soname at each release, though, I wouldn't get my hopes high. BTW it seems the dependents of libarchive use static libs instead of dlls. It's done to not bork a working installation, I guess?

@lazka
Copy link
Member

lazka commented Aug 16, 2021

libarchive/libarchive#1065 :D

@lazka
Copy link
Member

lazka commented Aug 16, 2021

So either we fix https://github.com/libarchive/libarchive/blob/2d57843d3c1bd6061df94d2554d3349ef60bf8e3/CMakeLists.txt#L74 to not add the minor version (it's pretty clear this is wrong as it is now imo), or we rebuild (bump pkgrel of all packages in this PR)

@ongun-kanat
Copy link
Author

So either we fix https://github.com/libarchive/libarchive/blob/2d57843d3c1bd6061df94d2554d3349ef60bf8e3/CMakeLists.txt#L74 to not add the minor version (it's pretty clear this is wrong as it is now imo), or we rebuild (bump pkgrel of all packages in this PR)

From libarchive/libarchive#1065 I infer that the soname in the CMake is the correct one. So every minor version is a soname change.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants