Repository navigation
[judge] Install RPATH of the libraries and Python modules, environment scripts without empty entries, unit_system in Python - #394
Open
trsxvz wants to merge 4 commits into
Conversation
The `MaterialProperty` class of `mgis.material_property` did not expose the `unit_system` member of the C++ structure (available since 3.1), so the unit system declared by `@UnitSystem` could not be read from Python. Decision: `unit_system` is exposed as a read-only attribute, placed and documented as the other members (`def_readonly`, same order as the C++ structure); this only adds an attribute, so it is non-breaking. The members and the `isGenericParallelMaterialProperty` predicate of the `generic-parallel` interface are not added: they belong to PR #321, which is not merged, and shall be exposed there. Alternatives: a writable attribute (`def_readwrite`), rejected since all the other members are read-only. Review item: M-093
The installed MGIS libraries and python modules had no RUNPATH: a program built against an installation, or the python modules, did not find the MGIS libraries and their dependencies (TFEL, HDF5) without setting LD_LIBRARY_PATH, and the loader could silently pick another MGIS library (for instance one installed in /usr/local/lib, which is in the loader cache): the C bindings found by the RUNPATH of a program then loaded the MGIS library of /usr/local/lib. Decision: the install rpath is set as TFEL does on macOS (lib directory of the installation prefix unless it is a system directory, plus the directories of the linked libraries through CMAKE_INSTALL_RPATH_USE_LINK_PATH), on all systems, with absolute paths as TFEL. It is non-breaking: RUNPATH comes after LD_LIBRARY_PATH, so with the env.sh scripts an installation, relocated or not, uses the same libraries as before; the values of CMAKE_INSTALL_RPATH and CMAKE_INSTALL_RPATH_USE_LINK_PATH given by the user (e.g. by spack) are kept; the standard CMAKE_SKIP_INSTALL_RPATH variable disables it (no new option). Alternatives: no install rpath on Linux (as TFEL), which leaves the defect; relative paths ($ORIGIN, @loader_path), which need a per-target rpath for the python modules and differ from TFEL. Review: found by the local integration test of the review
The env.sh script appended `:$LD_LIBRARY_PATH` (and `:$PYTHONPATH`,
`:$PATH`) even when the variable was empty or unset, leaving a trailing
empty entry, i.e. the current directory, in the search paths of the
loader and of Python: a library or a python module of the current
directory could be loaded. The sh and fish scripts also added `lib` to
LD_LIBRARY_PATH instead of `lib${LIB_SUFFIX}`, where the libraries are
installed.
Decision: the previous value is appended only if it is not empty
(`${VAR:+:$VAR}`; TFEL's env.sh has the same trailing `:`), and
`@LIB_SUFFIX@` is used as in TFEL's scripts. The scripts still do not
set the TFEL environment, as stated by the 3.2 release notes: sourcing
the env.sh of TFEL from a path fixed at configuration would break when
TFEL is moved, and the TFEL and HDF5 libraries used by the MGIS
libraries are now found through their install rpath. Non-breaking: the
non-empty entries and their order are unchanged.
Alternatives: sourcing TFEL's env.sh, or adding the TFEL and HDF5
library directories to LD_LIBRARY_PATH, in the MGIS scripts.
Review: found outside the review, while checking the installation
…ries-without-runpath-and-empty-entries-added-by-the-installed-environment-scripts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
[AI-assisted]
[judge]: setting an install RPATH and the form of the environment scripts are decisions about the installation; they are listed in "Decisions made" and left for review.
Fixes #393.
Refs #381.
One commit per problem.
Decisions made
unit_systemof a material property (Python bindings: MaterialProperty exposes neither the unit system nor the generic-parallel description #381, part available on master): exposed read-only in the PythonMaterialProperty, aftertfel_versionas in the C++ structure. The generic-parallel members of generic-parallel material properties: load, check and describe the libraries generated by TFEL 5.2 #321 are not added here (that pull request is not merged). Not taken: a writable attribute (every other member is read-only).CMAKE_INSTALL_RPATHis${prefix}/lib${LIB_SUFFIX}(left out for a system directory) andCMAKE_INSTALL_RPATH_USE_LINK_PATHadds the directories of the linked libraries (TFEL, HDF5). Values given by the user (Spack passes both) are kept, and the standardCMAKE_SKIP_INSTALL_RPATHdisables it, so no new option. Non-breaking: RUNPATH comes afterLD_LIBRARY_PATH, so an installation used withenv.shloads the same libraries as before. Not taken: no install RPATH on Linux, as TFEL (leaves the defect); relative paths ($ORIGIN,@loader_path), which need a separate RPATH for the Python modules and differ from TFEL.${VAR:+:$VAR}, so that the previous value is appended only when it is not empty (the non-empty entries and their order are unchanged);lib@LIB_SUFFIX@instead oflib, as the scripts of TFEL. The scripts still do not set the environment of TFEL, as the 3.2 release notes state. Not taken: sourcing theenv.shof TFEL from a path fixed at configure time (breaks when TFEL is moved); adding the TFEL and HDF5 directories toLD_LIBRARY_PATH.Tested locally (Ubuntu 24.04, g++ 13.3, TFEL master)
unit_system: not exposed on master;''forInconel600and'SI'for a material property with@UnitSystem SIwith this branch; read-only.readelf -d: no RUNPATH on the 3 libraries and 5 Python modules on master;<prefix>/lib:<tfel>/lib:<hdf5>/libwith this branch. With only the TFEL environment set, a C program, a Fortran program and the Python module loaded an older MGIS of/usr/local/lib(or failed) on master, and load the right one with this branch. TFEL's own libraries still need TFEL's environment (they have no RUNPATH on Linux).CMAKE_SKIP_INSTALL_RPATH=ON, user values, a/usrprefix andLIB_SUFFIX=64give the expected RPATH.env -i bashthensource <prefix>/share/mgis/env/env.sh:LD_LIBRARY_PATHandPYTHONPATHended with:on master, and Python imported a module planted in the current directory; with this branch no empty entry, the planted module is not found, and values set beforehand are kept.make check: 89/89 tests pass, as on master.