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

Support acquiring libmagic from vcpkg. #4119

Merged
merged 21 commits into from
Jul 18, 2023

Conversation

teo-tsirpanis
Copy link
Member

@teo-tsirpanis teo-tsirpanis commented Jun 6, 2023

SC-29902

This PR ports libmagic, the last of our external dependencies to have been exclusively available via ExternalProject, to vcpkg.

The upstream libmagic vcpkg port was unsuitable for our use because it uses autoconfig and make, and therefore does not expose CMake targets. For this reason I created a custom CMake-powered port, inspired from both the upstream port and our own fork of libmagic that we have been using.

In case regex.h is not available (namely on Windows), the upstream vcpkg port used the tre library. This vcpkg port always uses pcre2 for consistency, matching our fork's behavior. Pcre2's CMake integration had some problems so I added a custom port for it as well.

I have also opened PCRE2Project/pcre2#260 to fix pcre2's CMake integration upstream, and after that gets merged I plan to contribute these two custom ports to upstream vcpkg.

It builds locally on Linux; on Windows it has some issues that might or might not occur due to my local configuration. Let's see what CI thinks.


TYPE: IMPROVEMENT
DESC: Support acquiring libmagic from vcpkg.

@shortcut-integration
Copy link

This pull request has been linked to Shortcut Story #29902: Support acquiring libmagic from vcpkg..

@davisp
Copy link
Contributor

davisp commented Jun 6, 2023

Other than the random error on Azure's macOS runner this looks pretty good as far as I can tell. Theoretically I'll have a bit of time this afternoon to take a look and see if I can figure out the issue. Given that the two GHA macOS runners succeed I'm hopefully it'll be an easy fix.

@teo-tsirpanis
Copy link
Member Author

I think I found what causes the macOS failures.

Vcpkg normally (unless we tell it otherwise which we don't on macOS) builds the ports in both debug and release configurations. The configure step for both happens concurrently (unless we pass the DISABLE_PARALLEL_CONFIGURE option in vcpkg_cmake_configure) and each gets configured at a different binary directory.

However, libmagic auto-generates a file in the source directory, and writes to it gradually. When both debug and release configurations write to it at the same time, the file gets corrupted and the file tool fails to read it. We fix it by writing this file directly to the binary directory, isolating it from other concurrent configures. Other fixes would be to concatenate the files to a string buffer and write it at once or disabling concurrent configures in the portfile, but this is fixes the root cause of the problem.

@davisp
Copy link
Contributor

davisp commented Jun 6, 2023

@teo-tsirpanis Awesome find! I duplicated this locally by backing up to before the concurrent build fix, observing the build failure, then including the concurrent build fix and it compiles cleanly for me.

Copy link
Contributor

@davisp davisp left a comment

Choose a reason for hiding this comment

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

+1

@teo-tsirpanis
Copy link
Member Author

Can we merge it?

@davisp
Copy link
Contributor

davisp commented Jun 9, 2023

@ihnorton I'm gonna leave this decision to you. I'm happy with the PR, but I don't know enough about our downstream packaging/release processes to be certain there won't be an issue with any of that.

@teo-tsirpanis
Copy link
Member Author

Maybe it could not get into 2.16, if we are about to branch it off soon, but this is blocking further work on the build system and vcpkg.

@ihnorton ihnorton merged commit aa6a196 into TileDB-Inc:dev Jul 18, 2023
50 checks passed
@teo-tsirpanis teo-tsirpanis deleted the libmagic-vcpkg branch July 19, 2023 14:13
KiterLuc pushed a commit that referenced this pull request Mar 12, 2024
[SC-38521](https://app.shortcut.com/tiledb-inc/story/38521/update-libmagic-and-use-the-upstream-vcpkg-port)

Split from #4553.

This PR updates libmagic to version 5.45 and switches to using a vcpkg
port closer to the upstream one, which we can easily consume with
find_package(unofficial-libmagic) since microsoft/vcpkg#35274.

One complication is that the upstream port builds libmagic with its
official autotools-based build system, which is significantly slower on
Windows (on Linux it builds pretty fast). I tried to upstream the
CMake-based port I had added in #4119, but the PR was rejected.

Apart from binary caching, there is unfortunately nothing we can do
about the build performance regression. We could maintain the
CMake-based port for our own use, but it will split what we build and
what a future user of TileDB from vcpkg will build, and that port is not
without its problems (it failed for example when I tried cross-compiling
to arm64-windows, because it tried to execute the arm64 file.exe on my
x64 machine).

---
TYPE: BUILD
DESC: Update libmagic to version 5.45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants