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

expat: Fix cmake config files #128997

Merged
merged 2 commits into from
Jul 4, 2021

Conversation

OPNA2608
Copy link
Contributor

@OPNA2608 OPNA2608 commented Jul 2, 2021

Closes #128808
This is a universal fix for what the above PR was trying to implement locally.

Motivation for this change

As mentioned in #128808, the expat bump from #124212 introduced new, currently broken CMake files. This broke the freshly introduced opencolorio 2.x (part of #127522) and in turn our Blender package. This patches those files so everything works again.

Why are they broken?

expat.cmake:

# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "") 
endif()

# Create imported target expat::expat
add_library(expat::expat SHARED IMPORTED)

set_target_properties(expat::expat PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

# Load information for each installed configuration.
get_filename_component(_DIR "${CMAKE_CURRENT_LIST_FILE}" PATH)
file(GLOB CONFIG_FILES "${_DIR}/expat-*.cmake")
foreach(f ${CONFIG_FILES})
  include(${f})
endforeach()

expat-noconfig.cmake:

# Import target "expat::expat" for configuration ""
set_property(TARGET expat::expat APPEND PROPERTY IMPORTED_CONFIGURATIONS NOCONFIG)
set_target_properties(expat::expat PROPERTIES
  IMPORTED_LOCATION_NOCONFIG "${_IMPORT_PREFIX}/lib/libexpat.so.1.8.1"
  IMPORTED_SONAME_NOCONFIG "libexpat.so.1"
  )

list(APPEND _IMPORT_CHECK_TARGETS expat::expat )
list(APPEND _IMPORT_CHECK_FILES_FOR_expat::expat "${_IMPORT_PREFIX}/lib/libexpat.so.1.8.1" )

Since _IMPORT_PREFIX is calculated from the dev output that has the cmake files but the libraries are in the out output, the later check can never find the libraries. We'll replace the ${_IMPORT_PREFIX} usage in the library path with the correct output variable.

CC @hartwork, upstream expat developer & committer of the bump that introduced these files

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • 21.11 Release Notes (or backporting 21.05 Relase notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

@hartwork
Copy link
Contributor

hartwork commented Jul 2, 2021

@OPNA2608 the generated expat-noconfig.cmake file is identical with what CMake would produce. If there's a real bug here please report this upstream at https://github.com/libexpat/libexpat/issues and help me understand the issue better because I don't understand the problem, yet. Video calls in German or English are also an option. Thank you.

@OPNA2608
Copy link
Contributor Author

OPNA2608 commented Jul 2, 2021

The current problem is that expat's CMake files and headers get installed to a different prefix (/nix/store/*<hash-abc>-expat-2.4.1-dev*/{include,lib/cmake}) than the library files (/nix/store/*<hash-def>-expat-2.4.1*/lib/libexpat.so.<etc>). When CMake walks up the file tree from the location of the CMake files to get the _IMPORT_PREFIX, it's still under the dev prefix. That's the correct prefix for the include directory, but not for the library. As reported in #127522 this then causes opencolorio 2.x to FTBFS because the path /nix/store/*<hash-abc>-expat-2.4.1-dev*/lib/libexpat.so.<etc> that the expat CMake module returned doesn't exist.

@hartwork
Copy link
Contributor

hartwork commented Jul 2, 2021

@OPNA2608 is this an upstream bug or a nixOS packaging bug? If upstream, please help me reproduce it on a more mainstream flavor of Linux.

@OPNA2608
Copy link
Contributor Author

OPNA2608 commented Jul 2, 2021

I think this is a 50:50 situation. There may be a problem with the paths passed via the configure-based build system not being properly reflected in the final CMake modules. But some automatic NixOS packaging decisions screw with a different assumption. Consider this configuration:

./configure --prefix=$PWD/target-prefix --includedir=$PWD/include-prefix/include --libdir=$PWD/lib-prefix/lib

On a Ubuntu 18.04.5 system after make && make install, expat.cmake in $PWD/lib-prefix/lib/cmake/expat-2.4.1/expat.cmake reads:

# Compute the installation prefix relative to this file.
get_filename_component(_IMPORT_PREFIX "${CMAKE_CURRENT_LIST_FILE}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
get_filename_component(_IMPORT_PREFIX "${_IMPORT_PREFIX}" PATH)
if(_IMPORT_PREFIX STREQUAL "/")
  set(_IMPORT_PREFIX "") 
endif()

# Create imported target expat::expat
add_library(expat::expat SHARED IMPORTED)

set_target_properties(expat::expat PROPERTIES
  INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include"
)

Running the 4 get_filename_component calls on $PWD/lib-prefix/lib/cmake/expat-2.4.1/expat.cmake would give you the equivalent to $PWD/lib-prefix in _IMPORT_PREFIX. $PWD/lib-prefix/include doesn't match the --includedir=$PWD/include-prefix/include path specified via configure, so it would fail on finding the headers.

This is what would happen on NixOS as well, but the <libdir>/cmake directory gets moved to <includedir>/../lib/cmake post-installation, with the rationale that all the headers & build-related files should be under the same prefix. So the include directory is now found under the same prefix, but the libraries aren't.

The last path component of the --libdir makes it into the CMake files: --libdir=$PWD/lib-prefix/lib32 gives me IMPORTED_LOCATION_NOCONFIG "${_IMPORT_PREFIX}/lib32/libexpat.so.1.8.1".
The same doesn't apply to --includedir: --includedir=$PWD/include-prefix/include32 (nonsensical but still) results in the same INTERFACE_INCLUDE_DIRECTORIES "${_IMPORT_PREFIX}/include" as before.

I hope this helps you with reproducing the bug on your end.

@hartwork
Copy link
Contributor

hartwork commented Jul 2, 2021

@OPNA2608 thanks, I think the issue is clear now. Given the high complexity and effort for a clean fix upstream, I cannot promise a soon fix at the moment. Patching this for this in nixOS packaging is probably a good idea.

@cole-h
Copy link
Member

cole-h commented Jul 2, 2021

@ofborg eval

(Sorry, ofborg's certs recently had an issue, meaning that it was unable to
react to PRs. This has been fixed, but all PRs filed in this period will need
ofborg to be manually kicked off.)

OPNA2608 and others added 2 commits July 3, 2021 19:30
Header & library path constructions in CMake modules expect them to reside under the same prefix as the CMake files.
This assumption doesn't work with our multiple outputs so we patch the library path to the correct output.

Co-authored-by: Dmitry Kalinkin <dmitry.kalinkin@gmail.com>
@veprbl veprbl force-pushed the fix/expat-cmake-files/21.11 branch from ab86042 to 74af056 Compare July 3, 2021 23:30
Staging automation moved this from Needs review to Ready Jul 3, 2021
@veprbl veprbl merged commit 29aeda3 into NixOS:staging Jul 4, 2021
Staging automation moved this from Ready to Done Jul 4, 2021
@OPNA2608 OPNA2608 deleted the fix/expat-cmake-files/21.11 branch September 27, 2022 17:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Staging
  
Done
Development

Successfully merging this pull request may close these issues.

None yet

4 participants