Skip to content

[cmake] Do not vendor 2 lzma versions, but download one single source tarball #21423

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.

Sign up for GitHub

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

Open
dpiparo wants to merge 1 commit into
base: master
Choose a base branch
from
+31 −44
Open

[cmake] Do not vendor 2 lzma versions, but download one single source tarball #21423

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
75 changes: 31 additions & 44 deletions cmake/modules/SearchInstalledSoftware.cmake
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -328,58 +328,45 @@ if(NOT builtin_lzma)
endif()

if(builtin_lzma)
set(lzma_version 5.2.4)
set(lzma_version 5.8.2)
set(LZMA_TARGET LZMA)
message(STATUS "Building LZMA version ${lzma_version} included in ROOT itself")
if(WIN32)
set(lzma_version 5.6.3)
set(LIBLZMA_LIBRARIES ${CMAKE_BINARY_DIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}lzma${CMAKE_STATIC_LIBRARY_SUFFIX})
ExternalProject_Add(
LZMA
URL ${CMAKE_SOURCE_DIR}/core/lzma/src/xz-${lzma_version}.tar.gz
URL_HASH SHA256=b1d45295d3f71f25a4c9101bd7c8d16cb56348bbef3bbc738da0351e17c73317
INSTALL_DIR ${CMAKE_BINARY_DIR}
CMAKE_ARGS -G ${CMAKE_GENERATOR} -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}
-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=${CMAKE_CXX_FLAGS_RELWITHDEBINFO}
-DCMAKE_CXX_FLAGS_RELEASE=${CMAKE_CXX_FLAGS_RELEASE}
-DCMAKE_CXX_FLAGS_DEBUG=${CMAKE_CXX_FLAGS_DEBUG}
BUILD_COMMAND ${CMAKE_COMMAND} --build . --config $<CONFIG> --target liblzma
INSTALL_COMMAND ${CMAKE_COMMAND} --install . --config $<CONFIG> --component liblzma_Development
LOG_DOWNLOAD 1 LOG_CONFIGURE 1 LOG_BUILD 1 LOG_INSTALL 1 LOG_OUTPUT_ON_FAILURE 1
BUILD_IN_SOURCE 1
BUILD_BYPRODUCTS ${LIBLZMA_LIBRARIES}
TIMEOUT 600
)
set(LIBLZMA_INCLUDE_DIR ${CMAKE_BINARY_DIR}/include)
message(STATUS "Downloading and building LZMA version ${lzma_version}")

set(LIBLZMA_LIBRARIES ${CMAKE_BINARY_DIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}lzma${CMAKE_STATIC_LIBRARY_SUFFIX})

if(MSVC)
set(LIBLZMA_CXXFLAGS_RELEASE ${CMAKE_CXX_FLAGS_RELEASE})
set(LIBLZMA_CXXFLAGS_RELWITHDEBINFO ${CMAKE_CXX_FLAGS_RELWITHDEBINFO})
else()
if(CMAKE_CXX_COMPILER_ID MATCHES Clang)
set(LIBLZMA_CFLAGS "-Wno-format-nonliteral")
set(LIBLZMA_LDFLAGS "-Qunused-arguments")
elseif( CMAKE_CXX_COMPILER_ID STREQUAL Intel)
set(LIBLZMA_CFLAGS "-wd188 -wd181 -wd1292 -wd10006 -wd10156 -wd2259 -wd981 -wd128 -wd3179 -wd2102")
set(LIBLZMA_CXXFLAGS "-Wno-format-nonliteral")
endif()
if(CMAKE_OSX_SYSROOT)
set(LIBLZMA_CFLAGS "${LIBLZMA_CFLAGS} -isysroot ${CMAKE_OSX_SYSROOT}")
set(LIBLZMA_CXXFLAGS "${LIBLZMA_CXXFLAGS} -isysroot ${CMAKE_OSX_SYSROOT}")
endif()
set(LIBLZMA_LIBRARIES ${CMAKE_BINARY_DIR}/lib/${CMAKE_STATIC_LIBRARY_PREFIX}lzma${CMAKE_STATIC_LIBRARY_SUFFIX})
set(LIBLZMA_CFLAGS "${LIBLZMA_CFLAGS} -O3")
ExternalProject_Add(
LZMA
URL ${CMAKE_SOURCE_DIR}/core/lzma/src/xz-${lzma_version}.tar.gz
URL_HASH SHA256=b512f3b726d3b37b6dc4c8570e137b9311e7552e8ccbab4d39d47ce5f4177145
INSTALL_DIR ${CMAKE_BINARY_DIR}
CONFIGURE_COMMAND <SOURCE_DIR>/configure --prefix <INSTALL_DIR> --libdir <INSTALL_DIR>/lib
--with-pic --disable-shared --quiet
--disable-scripts --disable-xz --disable-xzdec --disable-lzmadec --disable-lzmainfo --disable-lzma-links
Comment on lines -372 to -373
Copy link
Contributor

@guitargeek guitargeek Mar 1, 2026

Choose a reason for hiding this comment

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

Do all of these options have CMake equivalents? They were probably introduced for a reason, I think we need to keep them (unless looked into it in detail and we concluded they are not necessary).

Copy link
Member Author

@dpiparo dpiparo Mar 2, 2026

Choose a reason for hiding this comment

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

For windows, we never cared.

Copy link
Member Author

@dpiparo dpiparo Mar 4, 2026

Choose a reason for hiding this comment

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

Translating some of the flags:

  • --disable-shared --> BUILD_SHARED_LIBS
  • --disable-scripts --> XZ_TOOL_SCRIPTS
  • --disable-xz --> XZ_TOOL_XZ
  • --disable-xzdec--> XZ_TOOL_XZDEC
  • --disable-lzma-dec --> XZ_TOOL_LZMADEC
  • --disable-lzmainfo --> XZ_TOOL_LZMAINFO
  • --disable-lzma-links --> XZ_TOOL_SYMLINKS and(?) XZ_TOOL_SYMLINKS_LZMA

Since everything works, I assume the static lib is built with position independent code, and
we do not need to deal with the translation of --with-pic.

Other useful to disable:

  • XZ_DOC
  • XZ_THREADS

CC=${CMAKE_C_COMPILER} CXX=${CMAKE_CXX_COMPILER} CFLAGS=${LIBLZMA_CFLAGS} LDFLAGS=${LIBLZMA_LDFLAGS}
LOG_DOWNLOAD 1 LOG_CONFIGURE 1 LOG_BUILD 1 LOG_INSTALL 1 LOG_OUTPUT_ON_FAILURE 1
BUILD_IN_SOURCE 1
BUILD_BYPRODUCTS ${LIBLZMA_LIBRARIES}
TIMEOUT 600
)
set(LIBLZMA_INCLUDE_DIR ${CMAKE_BINARY_DIR}/include)
set(LIBLZMA_CXXFLAGS "${LIBLZMA_CXXFLAGS} -O3")
set(LIBLZMA_CXXFLAGS_RELEASE "${CMAKE_CXX_FLAGS_RELEASE} ${LIBLZMA_CXXFLAGS}")
set(LIBLZMA_CXXFLAGS_RELWITHDEBINFO "${CMAKE_CXX_FLAGS_RELWITHDEBINFO} ${LIBLZMA_CXXFLAGS}")
Comment on lines +348 to +349
Copy link
Contributor

@guitargeek guitargeek Mar 1, 2026

Choose a reason for hiding this comment

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

We are composing here the default build flags for a given configuration of LIBLZMA out of the the default CXXFLAGS from ROOT and flags related to warnings or headers, which are build type independent. These should be better passed via CMAKE_CXX_FLAGS, and not CXX_BUILD_FLAGS_<config>. Because in the end, CMake will concatenate both anyway during the liblzma configuration.

Copy link
Member Author

@dpiparo dpiparo Mar 4, 2026

Choose a reason for hiding this comment

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

Good point.

endif()

ExternalProject_Add(
LZMA
URL https://root.cern/download/xz-${lzma_version}.tar.gz
URL_HASH SHA256=f21fdf2c1ee004de30ca40377a273e3369186e6b7ab7b50a410eaa2e2bbefafb
Comment on lines +354 to +355
Copy link
Member

@pcanal pcanal Mar 4, 2026

Choose a reason for hiding this comment

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

As a side note, the main (only?) advantage of moving the file from the repository to a web server (which requires an internet connection) is that in case one of xz version is compromised, we can remove it tar file from the server and prevent any version of ROOT that uses from building with the builtin xz . i.e. thus (correctly) forcing the user to use the system version (if any and hopefully it was updated)

INSTALL_DIR ${CMAKE_BINARY_DIR}
CMAKE_ARGS -G ${CMAKE_GENERATOR} -DCMAKE_INSTALL_PREFIX=${CMAKE_BINARY_DIR}
-DCMAKE_CXX_FLAGS_RELWITHDEBINFO=${LIBLZMA_CXXFLAGS_RELWITHDEBINFO}
-DCMAKE_CXX_FLAGS_RELEASE=${LIBLZMA_CXXFLAGS_RELEASE}
-DCMAKE_CXX_FLAGS_DEBUG=${LIBLZMA_CXXFLAGS_RELEASE}
Comment on lines +358 to +360
Copy link
Contributor

@guitargeek guitargeek Mar 1, 2026

Choose a reason for hiding this comment

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

Forwarding these DCMAKE_CXX_FLAGS_<config> flags is unnecessary on Linux/macOS. We are not messing with these CMake variables to begin with, it only happens on Windows: https://github.com/dpiparo/root/blob/master/cmake/modules/SetUpWindows.cmake#L48

Copy link
Member Author

@dpiparo dpiparo Mar 4, 2026

Choose a reason for hiding this comment

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

fair enough.

BUILD_COMMAND ${CMAKE_COMMAND} --build . --config $<CONFIG> --target liblzma
INSTALL_COMMAND ${CMAKE_COMMAND} --install . --config $<CONFIG> --component liblzma_Development
Copy link
Contributor

@guitargeek guitargeek Mar 1, 2026 •
edited
Loading

Choose a reason for hiding this comment

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

Different from MSVC, Ninja and Make are single-configuration generators, so the --config $<CONFIG> will not have no effect and is ignored. So the configuration (like Release, Debug, RelWithDebInfo) is not forwarded unless you use MSVC, which makes this code a bit misleading, considering also the DCMAKE_CXX_FLAGS_<config> flags above. I would rather put the --config $<CONFIG> in an extra-args variable.

Copy link
Member Author

@dpiparo dpiparo Mar 4, 2026

Choose a reason for hiding this comment

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

I do not understand, could you be more precise?

Copy link
Contributor

@guitargeek guitargeek Mar 4, 2026

Choose a reason for hiding this comment

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

I mean having --config $<CONFIG> on non-MSVC platforms can be misleading because it was no effect. But actually there is no problem to keep it (I wrote this confused comment before I verified that it's indeed harmless on Linux/macOS).

LOG_DOWNLOAD 1 LOG_CONFIGURE 1 LOG_BUILD 1 LOG_INSTALL 1 LOG_OUTPUT_ON_FAILURE 1
BUILD_IN_SOURCE 1
BUILD_BYPRODUCTS ${LIBLZMA_LIBRARIES}
TIMEOUT 600
)
set(LIBLZMA_INCLUDE_DIR ${CMAKE_BINARY_DIR}/include)

add_library(LibLZMA STATIC IMPORTED GLOBAL)
add_library(LibLZMA::LibLZMA ALIAS LibLZMA)
target_include_directories(LibLZMA INTERFACE ${LIBLZMA_INCLUDE_DIR})
Expand Down
Binary file removed core/lzma/src/xz-5.2.4.tar.gz
View file
Open in desktop
Binary file not shown.
Binary file removed core/lzma/src/xz-5.6.3.tar.gz
View file
Open in desktop
Binary file not shown.
Loading