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

[libmagic] Add CMake config. #35274

Merged
merged 6 commits into from
Dec 1, 2023
Merged

Conversation

teo-tsirpanis
Copy link
Contributor

  • Changes comply with the maintainer guide
  • SHA512s are updated for each updated download
  • The "supports" clause reflects platforms that may be fixed by this new version
  • Any fixed CI baseline entries are removed from that file.
  • Any patches that are no longer applied are deleted from the port's directory.
  • The version database is fixed by rerunning ./vcpkg x-add-version --all and committing the result.
  • Only one version is added to each modified port's versions file.

Copy link
Contributor

@dg0yt dg0yt left a comment

Choose a reason for hiding this comment

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

The config also needs to append to IMPORTED_CONFIGURATIONS.

ports/libmagic/unofficial-libmagic-config.cmake.in Outdated Show resolved Hide resolved
ports/libmagic/unofficial-libmagic-config.cmake.in Outdated Show resolved Hide resolved
ports/libmagic/unofficial-libmagic-config.cmake.in Outdated Show resolved Hide resolved
ports/libmagic/unofficial-libmagic-config.cmake.in Outdated Show resolved Hide resolved
ports/libmagic/unofficial-libmagic-config.cmake.in Outdated Show resolved Hide resolved
@teo-tsirpanis
Copy link
Contributor Author

Feedback addressed. I also set IMPORTED_LINK_INTERFACE_LANGUAGES_${config} when we have a static library.

@teo-tsirpanis teo-tsirpanis requested a review from dg0yt November 22, 2023 21:10
@teo-tsirpanis teo-tsirpanis requested a review from dg0yt November 22, 2023 23:15
@JonLiu1993 JonLiu1993 added the category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist label Nov 23, 2023
Comment on lines +69 to +74
include(CMakePackageConfigHelpers)
configure_package_config_file(
"${CMAKE_CURRENT_LIST_DIR}/unofficial-${PORT}-config.cmake.in"
"${CURRENT_PACKAGES_DIR}/share/unofficial-${PORT}/unofficial-${PORT}-config.cmake"
INSTALL_DESTINATION "share/unofficial-${PORT}"
)
Copy link
Contributor

Choose a reason for hiding this comment

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

FTR I believe that configure_package_config_file is intented for CMake project mode, but here it is used in script mode. Let's hope there are no future changes which break this use case.

@JonLiu1993
Copy link
Member

Tested usage successfully by libmagic:x64-windows triplet:

Total install time: 12 min
libmagic provides CMake targets:

    find_package(unofficial-libmagic REQUIRED)
    target_link_libraries(main PRIVATE unofficial::libmagic::libmagic)

The magic.mgc file can be accessed from the unofficial-libmagic_DICTIONARY variable.

@JonLiu1993 JonLiu1993 added the info:reviewed Pull Request changes follow basic guidelines label Nov 24, 2023
@teo-tsirpanis teo-tsirpanis requested a review from dg0yt November 28, 2023 13:04
@teo-tsirpanis
Copy link
Contributor Author

Is there anything left to do here?

@vicroms vicroms added the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Nov 29, 2023
@vicroms vicroms merged commit 16ee2ec into microsoft:master Dec 1, 2023
15 checks passed
@teo-tsirpanis teo-tsirpanis deleted the libmagic-config branch December 1, 2023 10:22
@JonLiu1993 JonLiu1993 removed the requires:vcpkg-team-review This PR or issue requires someone on the vcpkg team to take a further look. label Dec 4, 2023
KiterLuc pushed a commit to TileDB-Inc/TileDB 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
Labels
category:port-feature The issue is with a library, which is requesting new capabilities that didn’t exist info:reviewed Pull Request changes follow basic guidelines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants