Repository navigation
Conversation
…version> read by the CMake package
The installed MFrontGenericInterfaceConfig.cmake tested ENV{${TFEL_DIR}}
and ENV{${HDF5_DIR}}, i.e. the environment variable named by the value of
a variable that is empty in these branches, so the environment variables
TFEL_DIR and HDF5_DIR were never read and the build-time TFEL and HDF5
directories were always used. The branch on ENV{${VERSIONED_TFEL_HOME}}
was dead too: VERSIONED_TFEL_HOME was never defined by MGIS.
Decision: the environment variables are tested by name, and the name of
the versioned variable is built from the version of TFEL used to build
MGIS exactly as TFEL builds it in tfel-modules-common.cmake
(TFELHOME_<version> with '.' and '-' replaced by '_'), in a private
_VERSIONED_TFEL_HOME variable. Non-breaking: without these environment
variables the hints are unchanged, and the order of precedence of the
template (TFEL_DIR, ENV{TFEL_DIR}, TFELHOME, ENV{TFELHOME_<version>},
ENV{TFELHOME}) is kept.
Alternatives: dropping the versioned branch (it never worked), or reusing
the VERSIONED_TFEL_HOME variable that TFEL's own package files define,
which is only set once a TFEL package has been found, i.e. too late.
Review item: M-148
The only version file was installed as share/mgis/cmake/mgis-config-version.cmake,
a name that find_package never reads for the packages MFrontGenericInterface,
MFrontGenericInterface-c and MFrontGenericInterface-fortran, so every
find_package call with a version failed ("version: unknown") and
<package>_VERSION was never defined. It was also generated from VERSION,
a variable that is never set.
Decision: mgis_library writes and installs <name>ConfigVersion.cmake next to
<name>Config.cmake from PROJECT_VERSION, so the core, C and Fortran packages
all get one, and the orphan mgis-config-version.cmake is no longer
generated. The compatibility policy SameMinorVersion is kept: it is the one
of the previous file and of TFEL's version file (tfel.cmake), and the
libraries carry no SOVERSION while minor releases change the API (3.1
renamed Space.hxx). Non-breaking: find_package without a version is
unchanged, and requests of 3.1 or 3.1.0 now succeed.
Alternatives: SameMajorVersion (a request of 3.1 would then accept a later
3.x installation) or AnyNewerVersion, or keeping mgis-config-version.cmake,
which no package can read.
Review item: M-138
With enable-static=ON, MFrontGenericInterface-static did not receive the definitions of enable-exceptions, OpenMP::OpenMP_CXX (enable-openmp), MGIS_ADDITIONAL_LIBRARIES (TBB::tbb with enable-parallel-stl-algorithms) nor MGIS_ADDITIONAL_LINK_FLAGS (-fsycl, -stdpar): a program linked with the static library aborted on a contract violation where the shared library throws, and the static target did not carry TBB or OpenMP in its link interface. Decision: the four missing settings are added to the static target in the same blocks as the shared one, with the if(enable-static) branches already used for the other settings of this file. Non-breaking: nothing changes without enable-static, and the static library now follows the options of the build exactly as the shared library does. Alternatives: merging the shared and static settings into one loop over both targets (same result, larger diff, and the TFEL block needs its -static target selection anyway). Review item: M-140
…ed after MGIS_SUFFIX
With MGIS_APPEND_SUFFIX, the Python package and the Julia module were
installed in site-packages/mgis_ and julia/mgis_ whatever the suffix,
because MGIS_SUFFIX_FOR_PYTHON_MODULES and MGIS_SUFFIX_FOR_JULIA_MODULES
were never defined: two suffixed installations in the same prefix
overwrote each other's bindings. Besides, the modules of mgis.fenics
imported mgis.behaviour by its absolute name, which fails (or loads
another MGIS) once the package is renamed.
Decision: both variables are defined from MGIS_SUFFIX, with '.' and '-'
replaced by '_' as TFEL does for TFEL_SUFFIX_FOR_PYTHON_MODULES
(tfel-python-modules.cmake), in the if(MGIS_APPEND_SUFFIX) block of
mgis.cmake that already derives the suffixed CMake directory, so that
both the Python section and bindings/julia see them: MGIS_SUFFIX=3.2
gives the package mgis_3_2. The three modules of mgis.fenics import
behaviour relatively ("from .. import behaviour as mgis_bv"), which is
the same module for the unsuffixed package. Non-breaking: without
MGIS_APPEND_SUFFIX nothing changes, and with it the previous name mgis_
was not usable.
Alternatives: using MGIS_SUFFIX unchanged (a '.' or '-' is not valid in
a Python package name), or adding an option to keep the unsuffixed
name, as TFEL's unversioned-python-module-names (a new option).
Review item: M-130
… generator
With a multi-configuration generator (Visual Studio, Xcode, Ninja
Multi-Config), test_fortran_bindings and test_fenics_bindings used a
dedicated branch that was broken: the tests were not added to
build-tests, "CONFIGURATION ${conf}" was passed as arguments of the test
program (the keyword is CONFIGURATIONS), so each test was registered
once per configuration in every configuration, and set_tests_properties
received the environment as several values, so only the first one was
set (MGIS_TEST_BEHAVIOURS_LIBRARY and MGIS_TEST_MODELS_LIBRARY were
lost). The FEniCS branch also quoted MGIS_FENICS_TEST_DATA and pointed
it to a build directory without data.
Decision: the multi-configuration branches are dropped and the
single-configuration code is used with every generator, since
add_test(NAME ... COMMAND <target>) already resolves the executable of
the tested configuration. The C tests need no change: they have no
such branch, and the double PROPERTY of their Windows branches was
removed with the DEPENDS entries (M-153). Non-breaking: the tests
generated with a single-configuration generator are unchanged.
Alternatives: repairing the multi-configuration branches
(CONFIGURATIONS, one ENVIRONMENT list, add_dependencies), which keeps
a duplicate of the same code and the per-configuration test names.
Review item: M-129
…ke-packages-of-the-installation-environment-scripts-debug-flags-and-test-targets' into 370-cmake-packages-environment-variables-version-files-static-library-options-suffixed-modules-and-multi-configuration
…environment-variables-version-files-static-library-options-suffixed-modules-and-multi-configuration # Conflicts: # bindings/python/mgis/fenics/nonlinear_problem.py
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]
/!\ : TO MERGE ONLY AFTER MERGING #341
[judge]: each fix below needed a small decision that the defect alone does not settle. The decisions taken are listed in "Decisions made"; the default build and the default installation are unchanged. This branch was based on the branch of #341 (same lines); #341 is now merged, so the PR only shows its own commits (and the merge of the fix of the CUDA workflow of #341). This PR is left for review of these decisions.
Fixes #370.
One commit per problem.
Decisions made
ENV{TFEL_DIR}andENV{HDF5_DIR}are tested by name; the versioned branch uses the name built byTFELitself intfel-modules-common.cmake,TFELHOME_<version>with.and-replaced by_, from the version ofTFELused to build MGIS (private variable_VERSIONED_TFEL_HOME). The order of precedence is unchanged. Not taken: dropping the versioned branch (it never worked); usingVERSIONED_TFEL_HOMEofTFEL, defined only after aTFELpackage is found, i.e. too late.mgis_librarywrites and installs<name>ConfigVersion.cmakenext to each<name>Config.cmake(core, C and Fortran packages); the orphanmgis-config-version.cmakeis removed. The compatibility staysSameMinorVersion, the policy of the former file and ofTFEL(minor versions change the API, and the libraries have no SOVERSION). Not taken:SameMajorVersionorAnyNewerVersion; keeping the orphan file.if(enable-static)branches are added in place, as the existing ones (definitions of the exceptions,OpenMP::OpenMP_CXX,MGIS_ADDITIONAL_LIBRARIES,MGIS_ADDITIONAL_LINK_FLAGS). Not taken: one loop over both targets (same result, larger diff).MGIS_SUFFIX_FOR_PYTHON_MODULESandMGIS_SUFFIX_FOR_JULIA_MODULESare defined fromMGIS_SUFFIXwith.and-replaced by_, asTFELdoes, in the existingif(MGIS_APPEND_SUFFIX)block ofmgis.cmake(the package ismgis_3_2forMGIS_SUFFIX=3.2);mgis.fenicsimportsbehaviourrelatively. No new option. Not taken: the suffix unchanged (.and-are not valid in a Python package name); an option such asunversioned-python-module-namesofTFEL(new option).COMMANDis resolved per configuration). The C tests need no change (no such branch; their duplicatedPROPERTYwas removed by Build system: installed headers and CMake packages, OpenMP dependency, Debug flags, environment scripts and test targets #341). Not taken: repairing the multi-configuration branches (duplicated code and test names per configuration).No test is added. The release notes will be updated in a separate pull request gathering the issues fixed by the review.
Tested locally (Ubuntu 24.04, g++ 13.3, gfortran 13.3, CMake 3.28, Ninja Multi-Config, TFEL master), on top of #341:
TFEL_DIRandHDF5_DIRpointing to other installations are ignored before and used after;TFELHOME_5_2_0_devis ignored before and used after; without any variable, the installations used to build MGIS are found, as before.find_packageof the 3 packages with the versions 3.1, 3.1.0, 3.0, 3.2 and 4.0: every request with a version fails before ("version: unknown"); after, 3.1 and 3.1.0 are accepted (_VERSION3.1.0) and the others refused; the projects build and run.enable-exceptionsandenable-openmp: without the exceptions definitions and-fopenmpbefore (a program linked with it aborts), with them after (the exception is caught);TBB::tbblinked with the parallel STL.MGIS_APPEND_SUFFIXwithMGIS_SUFFIX=3.2-dev: modules installed inmgis_before,mgis_3_2_devafter;import mgis_3_2_dev.behaviourworks, andmgis_3_2_dev.fenics(with stand-ins ofdolfin,uflandffc) uses the suffixed behaviour module.build-tests, before; 17 Fortran tests with the full environment, built bybuild-testsand passing (ctest -C Release -R _f$: 17/17), after.ctest: 89/89 tests pass.References
if(https://cmake.org/cmake/help/latest/command/if.html):${}is evaluated beforeifreceives its arguments, soDEFINED ENV{${TFEL_DIR}}tests the variable named by the value;DEFINED ENV{TFEL_DIR}tests the environment variable.find_package, "Config Mode Version Selection" (https://cmake.org/cmake/help/latest/command/find_package.html): the version file must be next to the configuration file and named<config-file>Version.cmakeor<config-file>-version.cmake.add_test(https://cmake.org/cmake/help/latest/command/add_test.html) andset_tests_properties(https://cmake.org/cmake/help/latest/command/set_tests_properties.html): the keyword isCONFIGURATIONS, a target name given asCOMMANDis replaced by its location for every configuration, and the properties are given asPROPERTIES <prop1> <value1> ....