Skip to content

[llvm] Fix incorrect usage of LIBXML2_INCLUDE_DIRS in the Windows release script #95781

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 1 commit into from
Jun 17, 2024

Conversation

aganea
Copy link
Member

@aganea aganea commented Jun 17, 2024

Before this fix, when building the Windows LLVM package with the latest cmake 3.29.3 I was seeing:

C:\git\llvm-project>llvm\utils\release\build_llvm_release.bat --version 19.0.0 --x64 --skip-checkout --local-python
...
-- Looking for FE_INEXACT
-- Looking for FE_INEXACT - found
-- Performing Test HAVE_BUILTIN_THREAD_POINTER
-- Performing Test HAVE_BUILTIN_THREAD_POINTER - Failed
-- Looking for mach/mach.h
-- Looking for mach/mach.h - not found
-- Looking for CrashReporterClient.h
-- Looking for CrashReporterClient.h - not found
-- Looking for pfm_initialize in pfm
-- Looking for pfm_initialize in pfm - not found
-- Could NOT find ZLIB (missing: ZLIB_LIBRARY ZLIB_INCLUDE_DIR)
CMake Error at C:/Program Files/CMake/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:230 (message):
  Could NOT find LibXml2 (missing: LIBXML2_INCLUDE_DIR)
Call Stack (most recent call first):
  C:/Program Files/CMake/share/cmake-3.29/Modules/FindPackageHandleStandardArgs.cmake:600 (_FPHSA_FAILURE_MESSAGE)
  C:/Program Files/CMake/share/cmake-3.29/Modules/FindLibXml2.cmake:108 (FIND_PACKAGE_HANDLE_STANDARD_ARGS)
  cmake/config-ix.cmake:167 (find_package)
  CMakeLists.txt:921 (include)


-- Configuring incomplete, errors occurred!

It looks like LIBXML2_INCLUDE_DIRS (with the extra 'S') is a result variable that is set by cmake after a call to find_package(LibXml2). It is actually LIBXML2_INCLUDE_DIR (without the 'S') that shold be used as a input before the find_package call, since the 'S' variable is unconditionally overwritten, see https://github.com/Kitware/CMake/blob/master/Modules/FindLibXml2.cmake#L96. I am unsure exactly why that worked with older cmake versions.

@aganea
Copy link
Member Author

aganea commented Jun 17, 2024

@CarlosAlbertoEnciso Can you please take a look? It was this comment https://reviews.llvm.org/D129571#3651781 that triggered the change to LIBXML2_INCLUDE_DIRS.

@CarlosAlbertoEnciso
Copy link
Member

Looking at those original changes:

cmake -GNinja %cmake_flags% -DPYTHON_HOME=%python64_dir% -DPython3_ROOT_DIR=%python64_dir% -DLIBXML2_INCLUDE_DIR=%libxmldir%/include/libxml2 -DLIBXML2_LIBRARIES=%libxmldir%/lib/libxml2s.lib ..\llvm-project\llvm || exit /b

It is clear that the LIBXML2_INCLUDE_DIR is being set to the %libxmldir%/include/libxml2, which is what the patch is setting.

Copy link
Member

@CarlosAlbertoEnciso CarlosAlbertoEnciso left a comment

Choose a reason for hiding this comment

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

LGTM

Copy link
Collaborator

@zmodem zmodem left a comment

Choose a reason for hiding this comment

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

lgtm

@aganea aganea merged commit 8fe376f into llvm:main Jun 17, 2024
6 of 8 checks passed
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.

3 participants