add standard install locations #2278

untereiner · 2023-02-06T14:26:07Z

add GNUInstallDirs
install examples / scripts / schema to CMAKE_INSTALL_DATAROOTDIR (i.e. read-only architecture-independent data root (share))

TotoGaz

CMAKE_INSTALL_DATAROOTDIR is definitely better!

TotoGaz · 2023-02-07T04:37:22Z

src/CMakeLists.txt

+install( FILES ${SCHEMA_DIR}/schema.xsd
+         DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${CMAKE_PROJECT_NAME}/schema )


The creation of schema.xsd is done iff ENABLE_XML_UPDATES AND ENABLE_MPI AND UNIX AND NOT CMAKE_HOST_APPLE AND NOT ENABLE_CUDA evaluates to true. Otherwise we're using the last existing version of schema.xsd (yes, we commit generated code and I do agree that we shouldn't...).

Can you manage to reunite the creation of schema.xsd and its installation. I do not think we want a mismatch between geosx and its scheme.

Not sure I understand your point. The install is at the end outside the "if blabla then create schema" statement.
It should then install either the existing schema (the commited one) or either install the just generated one that crushed the commited one ?!

The root of the issue is that, yes, we commit the generated file schema.xsd, and I claim that it's a bad pattern (I think mostly driven by some readthedocs deployment considerations).

As such, there will always be a moment where the schema.xsd will be out of sync (Because someone will have forgotten to commit, for example. Or because the dev won't have ENABLE_XML_UPDATES set to true to save time. Or because some CLion remote working dev pattern won't update schema.xsd in your work tree, but only on the remote and you may not notice).

To enforce sync, we should only deploy the schema.xsd that has been generated by the geosx binary itself.
That way, our end-users will always have something reliable.

In that case any deployment of GEOSX on a GPU system will not have a schema installed... which is probably not what we want? Generation is only disabled with CUDA to avoid producing diffs on LvArray type names (something developers care about), but users need the schema regardless (whether freshly generated or pre-committed) for input validation. I agree that neither solution is perfect and we should ideally improve the schema generation to not produce diffs between systems/configurations.

In that case any deployment of GEOSX on a GPU system will not have a schema installed... which is probably not what we want?

What I do not want is that end-users get provided wrong information. It's worse than everything, because they may be building their workflow on a brittle tool, and this always gets back to us as a broken feature and we have to fix it quick. And we do not have the resources for this.
Also, having brittle side-tools can ruin the confidence users would place in geosx. I do not think we need this.

Generation is only disabled with CUDA to avoid producing diffs on LvArray type names (something developers care about), but users need the schema regardless (whether freshly generated or pre-committed) for input validation.

Then if the schema.xsd gets generated nevertheless in the installation folder but not into the git working tree, this should be fine? End-users would not care about the LvArray discrepancy? And this would generate no diff in git.

I'm simply saying that the committed schema.xsd cannot be considered as a trustworthy source of information.
I'd like geosx to produce it for consistency.
As such I'm fine having an installation step which basically does geosx -s > /path/to/install/schema.xsd.

I agree that neither solution is perfect and we should ideally improve the schema generation to not produce diffs between systems/configurations.

I think that the whole generated-doc-in-the-git pattern should be revised.
If the only reason why we do this is readthedocs, then we should bundle the generated doc and get readthedocs download and deploy it.

What I agree with you is that's a bad pattern to commit files that can be generated.
For me it is to a specific CI step to generate the documentation files and push them into a documentation git repository for readthedocs to find its files. I have each time a bunch of rst files to commit without really understanding the reason. It gets the commits messy IHMO.

For the time being I will follow your idea: add a specific cmake target that generates always the schema to install it

TotoGaz · 2023-02-07T17:07:05Z

Please don't force push when a review has started: it makes impossible to grab the increments which were provided after the review.
Also please to no push any PR to the main dashboard, it's only for issues. If you're working on issues related to your PR (the practice we want to promote), please do not forget to update the status to In progress/In review

Nevertheless, IIUC the problem is still there: we risk deploying a schema.xsd which can (and therefore will be some day) out of sync. We should avoid that.

TotoGaz · 2023-03-03T17:11:24Z

src/CMakeLists.txt

@@ -129,9 +130,10 @@ set_target_properties( geosx PROPERTIES INSTALL_RPATH_USE_LINK_PATH TRUE )

 install(TARGETS geosx RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)

+set(SCHEMA_DIR ${CMAKE_SOURCE_DIR}/coreComponents/schema)


Why do you extract SCHEMA_DIR from the next if scope?
It looks to me that it's not used outside of this scope?

you are true, previous attempt I didn't rollback

TotoGaz · 2023-03-03T17:36:26Z

src/CMakeLists.txt

+add_custom_target( geosx_generate_install_schema
+         ALL
+         COMMAND geosx -s ${CMAKE_BINARY_DIR}/schema.xsd >generate_schema.log 2>&1 || (cat generate_schema.log && exit 1)
+         WORKING_DIRECTORY ${CMAKE_BINARY_DIR}
+         DEPENDS geosx
+         COMMENT "Generating XML schema to install"
+         )


I confess I was kind of fearing this: we do not have any GPU on travis so the CUDA drivers are not installed and GEOSX cannot be run:

[1002/1083] Generating XML schema to install FAILED: CMakeFiles/geosx_generate_install_schema /tmp/build/CMakeFiles/geosx_generate_install_schema cd /tmp/build && /tmp/build/bin/geosx -s /tmp/build/schema.xsd >generate_schema.log 2>&1 || ( cat generate_schema.log && exit 1 ) GEOSX version: 0.2.0 - c++ compiler: clang 10.0.0 - cuda compiler version: 11.2 - MPI version: Open MPI v4.0.3, package: Debian OpenMPI, ident: 4.0.3, repo rev: v4.0.3, Mar 03, 2020 - HDF5 version: 1.12.1 ... CUDA ERROR (code = 35, CUDA driver version is insufficient for CUDA runtime version) at general.c:253

As options, we could

Add a cmake option to control this cmake step. That implies to update travis as well, and I'm not sure this will be seamless to change the option in the host-configs only on travis.

Ignore the error code (not my favorite option, obviously).

Check for the TRAVIS environment variable to be present and set to true (see here), and deploy the schema accordingly.

Define a cmake option to control this step, set to true by default for everyone, but overwritten to false if TRAVIS exists and is set to true.

If we chose the cmake option way, we'll surely have to update the doc as well.
(Not a big deal, but still needs to be done).

This problem is CI related. We should not put a cmake ghost argument, CI specific, to resolve this CI issue IHMO.
What do you think of passing the option --build-exe-only to the travis_build_and_test.sh script in case of GPU related builds ? (as the green target Ubuntu CUDA debug (20.04, clang 10.0.0 + gcc 9.4.0, open-mpi 4.0.3, cuda-11.2.152))

It's obviously better to have the options properly defined in the build.
Here the option would be some --deploy-scheme. If you manage to make it happen, that's great.

I do not think we can rely on --build-exe-only: knowing that the tests can be built is important, even if we can't run them.

Do you mean that the tests are build with option --build-exe-only ? If I understand the CMakeLists correctly they are build with the all target.
What you want is to be able to (compile only the executable or executable and tests) and choose to install or not the schema as a separate step. Default behavior would install the schema. That's not so easy to implement only passing arguments to cmake to configure step.

Since you want to keep the default behavior and their isn't a global target to build the tests and I should not list all tests as build targets in the travis_build_and_test.sh a simple way is to exclude from all the schema generation target when specified by the user.

If it works with GEOSX_INSTALL_SCHEMA I will document the variable as well

scripts/travis_build_and_test.sh

src/CMakeLists.txt

.travis.yml

TotoGaz · 2023-03-08T16:30:21Z

.travis.yml

@@ -187,7 +187,7 @@ jobs:
    env:
    - DOCKER_REPOSITORY=geosx/centos7.6.1810-gcc8.3.1-cuda10.1.243
    - CMAKE_BUILD_TYPE=Release
-    - BUILD_AND_TEST_ARGS="--disable-unit-tests --reduce-install-logs --disable-schema-deployment"
+    - BUILD_AND_TEST_ARGS="--disable-unit-tests --disable-schema-deployment"


I think you forgot to remove https://github.com/GEOSX/GEOSX/blob/b6b0e875d1b55bca361497bb572ef83a25c31f65/scripts/travis_build_and_test.sh#L83-L87 which lead me to --reduce-install-logs being dummy.

TotoGaz · 2023-03-08T16:30:37Z

scripts/travis_build_and_test.sh

-
-
-exit 0
+exit 0


Suggested change

exit 0

exit 0

TotoGaz · 2023-03-08T16:30:49Z

scripts/travis_build_and_test.sh

 if [[ "$*" == *--disable-schema-deployment* ]]; then
-  GEOSX_INSTALL_SCHEMA=0
+  GEOSX_INSTALL_SCHEMA=0  


Suggested change

GEOSX_INSTALL_SCHEMA=0

GEOSX_INSTALL_SCHEMA=0

TotoGaz · 2023-03-08T16:35:03Z

src/docs/doxygen/GeosxConfig.hpp

@@ -114,10 +114,10 @@
 /* #undef chai_VERSION */

 /// Version information for adiak
-#define adiak_VERSION ..
+/* #undef adiak_VERSION */


Please rollback this file. I know it's generated automatically... Sorry for this.

Have'nt seen it. should avoit git commit -a

TotoGaz · 2023-03-08T20:28:00Z

scripts/travis_build_and_test.sh

@@ -57,7 +61,8 @@ or_die python3 scripts/config-build.py \
               -bp ${GEOSX_BUILD_DIR} \
               -ip ${GEOSX_DIR} \
               --ninja \
-               -DBLT_MPI_COMMAND_APPEND='"--allow-run-as-root;--oversubscribe"'
+               -DBLT_MPI_COMMAND_APPEND='"--allow-run-as-root;--oversubscribe"' \
+               -DGEOSX_INSTALL_SCHEMA=${GEOSX_INSTALL_SCHEMA}


In the logs I find

[config-build.py]:[INFO]: Passing the following unknown arguments directly to cmake: ['-DBLT_MPI_COMMAND_APPEND="--allow-run-as-root;--oversubscribe"', '-DGEOSX_INSTALL_SCHEMA=']

whatever if you've added --disable-schema-deployment or not. Can you double check?

I initialized the variable with 1 so there are -DGEOSX_INSTALL_SCHEMA=0 and -DGEOSX_INSTALL_SCHEMA=1

untereiner self-assigned this Feb 6, 2023

untereiner linked an issue Feb 6, 2023 that may be closed by this pull request

XSD file in installations #2228

Closed

untereiner requested a review from TotoGaz February 6, 2023 14:27

untereiner added the type: feature New feature or request label Feb 6, 2023

TotoGaz reviewed Feb 7, 2023

View reviewed changes

untereiner added 2 commits February 7, 2023 09:38

add GNUInstallDirs

53f4bca

SCHEMA_DIR used for either existing and generated schema

8552216

untereiner force-pushed the feature/untereiner/install branch from da2b8e9 to 8552216 Compare February 7, 2023 08:38

add new target to generate schema into build tree before install it

d9f7ac3

untereiner requested review from TotoGaz and klevzoff March 3, 2023 08:08

TotoGaz reviewed Mar 3, 2023

View reviewed changes

untereiner requested a review from TotoGaz March 6, 2023 15:57

untereiner added 2 commits March 7, 2023 12:57

test with exclude from all

7206676

add optional keyword to skip install

2c1eab2

TotoGaz reviewed Mar 8, 2023

View reviewed changes

scripts/travis_build_and_test.sh Outdated Show resolved Hide resolved

scripts/travis_build_and_test.sh Outdated Show resolved Hide resolved

src/CMakeLists.txt Show resolved Hide resolved

.travis.yml Outdated Show resolved Hide resolved

untereiner added 2 commits March 8, 2023 09:41

add new option to CMakeLists

f0cbdd4

update following comments

5881bcc

untereiner requested a review from TotoGaz March 8, 2023 13:02

TotoGaz reviewed Mar 8, 2023

View reviewed changes

cosmetics

4a2c570

untereiner requested a review from TotoGaz March 8, 2023 17:29

TotoGaz reviewed Mar 8, 2023

View reviewed changes

init shell script variable

4de9b7c

untereiner requested a review from TotoGaz March 13, 2023 15:40

TotoGaz approved these changes Mar 13, 2023

View reviewed changes

CusiniM merged commit de32622 into develop Mar 14, 2023

CusiniM deleted the feature/untereiner/install branch March 14, 2023 22:54

TotoGaz mentioned this pull request Apr 25, 2023

Update Schema Installation #2408

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

add standard install locations #2278

add standard install locations #2278

untereiner commented Feb 6, 2023

TotoGaz left a comment

TotoGaz Feb 7, 2023

untereiner Mar 1, 2023 •

edited

Loading

TotoGaz Mar 1, 2023

klevzoff Mar 1, 2023

TotoGaz Mar 1, 2023

untereiner Mar 2, 2023

TotoGaz commented Feb 7, 2023

TotoGaz Mar 3, 2023

untereiner Mar 6, 2023

TotoGaz Mar 3, 2023

untereiner Mar 6, 2023

TotoGaz Mar 6, 2023

untereiner Mar 7, 2023 •

edited

Loading

TotoGaz Mar 8, 2023

untereiner Mar 8, 2023

TotoGaz Mar 8, 2023

TotoGaz Mar 8, 2023

TotoGaz Mar 8, 2023

untereiner Mar 8, 2023 •

edited

Loading

TotoGaz Mar 8, 2023

untereiner Mar 13, 2023

		install( FILES ${SCHEMA_DIR}/schema.xsd
		DESTINATION ${CMAKE_INSTALL_DATAROOTDIR}/${CMAKE_PROJECT_NAME}/schema )

		@@ -129,9 +130,10 @@ set_target_properties( geosx PROPERTIES INSTALL_RPATH_USE_LINK_PATH TRUE )

		install(TARGETS geosx RUNTIME DESTINATION ${CMAKE_INSTALL_PREFIX}/bin)

		set(SCHEMA_DIR ${CMAKE_SOURCE_DIR}/coreComponents/schema)

add standard install locations #2278

add standard install locations #2278

Conversation

untereiner commented Feb 6, 2023

TotoGaz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untereiner Mar 1, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

TotoGaz commented Feb 7, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untereiner Mar 7, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untereiner Mar 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

untereiner Mar 1, 2023 •

edited

Loading

untereiner Mar 7, 2023 •

edited

Loading

untereiner Mar 8, 2023 •

edited

Loading