Skip to content

[libjxl] Remove libm dependency when building with MSVC #46177

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

Merged
merged 2 commits into from
Jun 27, 2025

Conversation

ayeteadoe
Copy link
Contributor

@ayeteadoe ayeteadoe commented Jun 26, 2025

This patches MSVC builds to not link with libm. Otherwise, when NOT BUILD_SHARED_LIBS is true, targets that depend on the libjxl target try to link with m.lib.

The following was the previous workaround I had to do before this fix:

diff --git a/Libraries/LibGfx/CMakeLists.txt b/Libraries/LibGfx/CMakeLists.txt
index 794271d014..2c55c49dfd 100644
--- a/Libraries/LibGfx/CMakeLists.txt
+++ b/Libraries/LibGfx/CMakeLists.txt
@@ -137,6 +137,11 @@ endif()
 
 if (NOT ANDROID)
     pkg_check_modules(Jxl REQUIRED IMPORTED_TARGET libjxl)
+    if (MSVC)
+        get_target_property(LIBJXL_INTERFACE_LINK_LIBS PkgConfig::Jxl INTERFACE_LINK_LIBRARIES)
+        list(REMOVE_ITEM LIBJXL_INTERFACE_LINK_LIBS "m")
+        set_target_properties(PkgConfig::Jxl PROPERTIES INTERFACE_LINK_LIBRARIES "${LIBJXL_INTERFACE_LINK_LIBS}")
+    endif()
     target_link_libraries(LibGfx PRIVATE PkgConfig::Jxl)
 else()
     find_package(libjxl REQUIRED)
  • 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.

@ayeteadoe
Copy link
Contributor Author

Is it possible to add a test for this?

@ayeteadoe
Copy link
Contributor Author

@microsoft-github-policy-service agree

@BillyONeal
Copy link
Member

The build failures appear legitimate. Perhaps the tools feature should be "supports": "native" or "supports": "!uwp"

Mengna-Li
Mengna-Li previously approved these changes Jun 27, 2025
@Mengna-Li Mengna-Li added the category:port-bug The issue is with a library, which is something the port should already support label Jun 27, 2025
Copy link
Member

@BillyONeal BillyONeal left a comment

Choose a reason for hiding this comment

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

Thanks for the fix!

-else()
- set(JPEGXL_REQUIRES_TYPE "Requires")
- set(JPEGXL_PUBLIC_LIBS "-lm ${PKGCONFIG_CXX_LIB}")
+# MSVCRT bundles math functions
Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

IMO the change is incorrect: JPEGXL_REQUIRES_TYPE should be set regardless of MSVC.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Copy link
Member

Choose a reason for hiding this comment

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

IMO the change is incorrect: JPEGXL_REQUIRES_TYPE should be set regardless of MSVC.

Sigh; you're right and I missed this :/

Copy link
Member

Choose a reason for hiding this comment

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

@BillyONeal BillyONeal merged commit 0cb95c8 into microsoft:master Jun 27, 2025
18 checks passed
@ayeteadoe ayeteadoe deleted the jxl-msvc-remove-libm branch June 27, 2025 21:46
@dc6c
Copy link

dc6c commented Jul 22, 2025

Unfortunately, this also needs to patch up lib/jxl_cms.cmake and lib/threads/libjxl_threads.pc.in, or otherwise the linking against libm sneaks in via those routes.

talregev pushed a commit to talregev/vcpkg that referenced this pull request Aug 11, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
category:port-bug The issue is with a library, which is something the port should already support
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants