Framework: Add ability to use explicit configure cachefile #12291
Conversation
This file contains 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
We need to pass -DTrilinos_CONFIGURE_OPTIONS_FILE=XXX instead of -C XXX sometimes. Decided to do it for any GCC build. User Support Ticket(s) or Story Referenced: trilinos#12024
sebrowne
added
AT: AUTOMERGE
sebrowne
requested review from
bartlettroscoe,
srbdev,
fryeguy52,
cabejackson and
jmlapre
User Support Ticket(s) or Story Referenced: TRILFRAME-596
sebrowne
added
the
AT: PRE-TEST INSPECTED
|
I'm expecting to have to change some of the unit tests for the PR code, but they're a pain to figure out how to run (I'm too lazy to configure Trilinos and run them with ctest), so I'll see what happens here. |
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
|
Looking at https://trilinos-cdash.sandia.gov/build/1140566/configure, I think there's an issue with how Trilinos_CONFIGURE_OPTIONS_FILE is working (or I expected it to work differently than it did). Will work on this later this morning. |
@sebrowne, there are a few problems here. First, you can see that the
And you see that CMake is processing the There is obviously a defect in this software. Is there no way to test this software locally without hammering the PR testing system? (It is easy to test the TriBITS CTest Driver locally and work out issues like this.) Second, my guess is that the configure output line: is getting generated from the CMake command-line arguments: For some reason, my guess is that CMake is interpreting that as: CMake and CTest are very tricky in how they handle quotes. My recommendation is to not use quotes in CMake command-line arguments and just leave them out (and hope there are no spaces in the file path names). How the CMake language handles data involving spaces, quotes, and semi-colons in the most serious and unfixable parts of the CMake language (and hence the need for a new CMake language). Welcome to CMake hell :-) |
|
Agreed, I saw that concatenated output. Thanks for the ideas on the command-line parsing, I think you're probably correct. I can test this locally myself, will do that later today. I intentionally passed the packageEnables.cmake as a -C option still. Do we want BOTH of them passed as the new option? I figured the packageEnables.cmake wouldn't be necessary for the testing system because it build specific packages as part of the test, but I'll admit that I didn't give it a wealth of thought up front. |
@sebrowne, right, my bad, that is a different file with a different purpose. In the context of the TrilinosInstallTesting reduced tarball tests, you should not have to pass Trilinos/packages/TrilinosInstallTests/CMakeLists.txt Lines 107 to 123 in 8b7ec4d So I think passing |
|
Okay I figured it out. There is some conjecture here but I'm 99% sure I know what happened. The quotation marks are included in the string when this is run, which tricks TriBITS into thinking that the path to the file is relative, which is then shown when it tries to load that file in the configure output. I replicated this locally. I will change the code to stop wrapping with escaped quotes (also I don't really think we need them). |
Having these here ends up with them in the value once we enter configure, which then confuses TriBITS about whether a path is relative or absolute. Not sure if this is the best place to remove them, but it's the easiest. User Support Ticket(s) or Story Referenced: TRILFRAME-596
|
Status Flag 'Pre-Test Inspection' - Auto Inspected - Inspection is Not Necessary for this Pull Request. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
Okay, that does make sense. Note that one way that you can include quotes is to quote the entire argument like: But again, CMake is very tricky with quotes so, when in doubt, leave them out. |
@sebrowne, FYI, it is not TriBITS that is taking the CMake input: and turning it into: It is actually raw CMake in the Trilinos/cmake/tribits/core/package_arch/TribitsGlobalMacros.cmake Lines 170 to 172 in 87d0c5c To explain this behavior, see: which states:
So it is raw CMake that thinks that a path that starts with the char to give: This just goes to show, most issues with TriBITS are actually just misunderstandings of how raw CMake works. If It said it 100 times I will say it again, be very careful with how quotes are used with CMake. It is very very confusing. |
|
Fair enough. I'm no stranger to escaped/non-escaped quotation nightmares with layers and layers of shell and Python scripts, so this is unsurprising regardless. I think the autotester was down when the last set of tests ran. Retesting. |
sebrowne
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
|
@sebrowne, should I go ahead and put in the exclude removal: for the file cmake/CallbackDefineRepositoryPackaging.cmake? I can test this locally but I believe that will resolve this problem noted above. Also, should I strengthen the test to ensure that the same set of enabled packages is produced in the untarred reduced source tree? (That would have caught this issue and will catch all future issues.) Given this example, I am nervous about the strength of this reduced tarball test. NOTE: We can make this change in a follow-up PR to allow this PR to merge (since some tarball testing is better than no tarball testing). |
|
NOTICE: The AutoTester has encountered an internal error (usually a Communications Timeout), testing will be restarted, previous tests may still be running but will be ignored by the AutoTester... |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: 1 or more Jobs FAILED Note: Testing will normally be attempted again in approx. 2 Hrs 30 Mins. If a change to the PR source branch occurs, the testing will be attempted again on next available autotester run. Pull Request Auto Testing has FAILED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
|
@bartlettroscoe I think strengthening the test as mentioned with the package asserts is a wise idea if it's not too costly. Why not restructure ShyLU to be TriBITS compliant? Too much work for this small of a stimulus given the payoff? I'm fine with whichever approach you prefer on this one. I'm going to AUTOMERGE this and mark it for one last cycle of testing given the current improvements unless you see any remaining issues. The last autotester run seems to have encountered network difficulties. |
sebrowne
added
the
AT: RETEST
|
Status Flag 'Pull Request AutoTester' - User Requested Retest - Label AT: RETEST will be reset after testing. |
|
Status Flag 'Pull Request AutoTester' - Testing Jenkins Projects: Pull Request Auto Testing STARTING (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
Using Repos:
Pull Request Author: sebrowne |
|
Status Flag 'Pull Request AutoTester' - Jenkins Testing: all Jobs PASSED Pull Request Auto Testing has PASSED (click to expand)Build InformationTest Name: Trilinos_PR_gcc-8.3.0
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-serial
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_gcc-8.3.0-debug
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_clang-11.0.1
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_python3
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_cuda-11.4.2-uvm-off
Jenkins Parameters
Build InformationTest Name: Trilinos_PR_intel-2021.3
Jenkins Parameters
|
trilinos-autotester
removed
the
AT: RETEST
|
Status Flag 'Pre-Merge Inspection' - - This Pull Request Requires Inspection... The code must be inspected by a member of the Team before Testing/Merging |
|
All Jobs Finished; status = PASSED, However Inspection must be performed before merge can occur... |
|
An approving review will schedule this to be merged @trilinos/framework @bartlettroscoe |
Should not take too long. (Might require a minor refactoring in TriBITS to build the "Final set of enabled [top-level] packages: ..." as a string to use in the regex to match for the test.)
Long story, much of which is discussed in: Short story ... The ShyLU subpackages have a circular dependency with Zoltan2 and therefore ShyLU had to be broken into two TriBITS packages: ShyLU_Node and ShyLU_DD. Then they created a pseudo TriBITS package ShyLU so that users could set
I will approve this PR so it can merge as is. I will put in a new PR to address the problem spelled out above. |
bartlettroscoe
approved these changes
Oct 3, 2023
There was a problem hiding this comment.
While not perfect (see above), this PR puts some automated reduced tarball creation testing in place.
I will put in a new PR later to address those issues with ShyLU and improve the strength of the testing.
|
Status Flag 'Pre-Merge Inspection' - SUCCESS: The last commit to this Pull Request has been INSPECTED AND APPROVED by [ bartlettroscoe ]! |
|
Status Flag 'Pull Request AutoTester' - Pull Request will be Automerged |
|
Merge on Pull Request# 12291: IS A SUCCESS - Pull Request successfully merged |
trilinos-autotester
removed
the
AT: AUTOMERGE
|
I added some instructions for how to replicate builds using this option on https://github.com/trilinos/Trilinos/wiki/Reproducing-PR-Testing-Errors |
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Oct 9, 2023
This adds a failing test for the call of tribits_disable_optional_dependency() discovered while testing the Trilinos PR trilinos/Trilinos#12291. Some other changes as part of this: * Add optional argument PACKAGE_NAME <packageName> to macro tribits_disable_optional_dependency() to allow the disable of a subpackage's dependency from its parent package's CMakeList.txt file. * Added new function tribits_get_have_package_dependency_macro_name(...) to eliminate code duplication for the naming of the HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_UC> macro.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Oct 9, 2023
This adds a failing test for the call of tribits_disable_optional_dependency() discovered while testing the Trilinos PR trilinos/Trilinos#12291. Some other changes as part of this: * Add optional argument PACKAGE_NAME <packageName> to macro tribits_disable_optional_dependency() to allow the disable of a subpackage's dependency from its parent package's CMakeList.txt file. * Added new function tribits_get_have_package_dependency_macro_name(...) to eliminate code duplication for the naming of the HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_UC> macro.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Oct 9, 2023
…/undefined packages (TriBITSPub#63) This is a better solution to the failure reported in trilinos/Trilinos#12291 and applied in the reverted commit: 05d4429 "Only assert defined vars conditionally" Author: Samuel Browne <[email protected]> Date: Thu Sep 28 13:51:05 2023 -0600 (6 days ago) (which I had suggested at the time). This new implementation sets HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_NAME_UC>=OFF at the project level right away when the dependencies are being read in and <upstreamPackageName> is not defined. This makes for more robust downstream CMake code that works with optional upstream dependencies when those dependencies are not defined (like when the package <upstreamPackageName> was removed from the reduced source tree by the reduced tarball feature). A downstream package should not care whether an upstream dependence is disabled or is missing entirely; the reaction should be the same and it should be able to rely on the same variables.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Oct 9, 2023
This adds a failing test for the call of tribits_disable_optional_dependency() discovered while testing the Trilinos PR trilinos/Trilinos#12291. Some other changes as part of this: * Add optional argument PACKAGE_NAME <packageName> to macro tribits_disable_optional_dependency() to allow the disable of a subpackage's dependency from its parent package's CMakeList.txt file. * Added new function tribits_get_have_package_dependency_macro_name(...) to eliminate code duplication for the naming of the HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_UC> macro.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Oct 9, 2023
…/undefined packages (TriBITSPub#63) This is a better solution to the failure reported in trilinos/Trilinos#12291 and applied in the reverted commit: 05d4429 "Only assert defined vars conditionally" Author: Samuel Browne <[email protected]> Date: Thu Sep 28 13:51:05 2023 -0600 (6 days ago) (which I had suggested at the time). This new implementation sets HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_NAME_UC>=OFF at the project level right away when the dependencies are being read in and <upstreamPackageName> is not defined. This makes for more robust downstream CMake code that works with optional upstream dependencies when those dependencies are not defined (like when the package <upstreamPackageName> was removed from the reduced source tree by the reduced tarball feature). A downstream package should not care whether an upstream dependence is disabled or is missing entirely; the reaction should be the same and it should be able to rely on the same variables.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Jan 26, 2024
This adds a failing test for the call of tribits_disable_optional_dependency() discovered while testing the Trilinos PR trilinos/Trilinos#12291. Some other changes as part of this: * Add optional argument PACKAGE_NAME <packageName> to macro tribits_disable_optional_dependency() to allow the disable of a subpackage's dependency from its parent package's CMakeList.txt file. * Added new function tribits_get_have_package_dependency_macro_name(...) to eliminate code duplication for the naming of the HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_UC> macro.
bartlettroscoe
added a commit
to bartlettroscoe/TriBITS
that referenced
this pull request
Jan 26, 2024
…/undefined packages (TriBITSPub#63) This is a better solution to the failure reported in trilinos/Trilinos#12291 and applied in the reverted commit: 05d4429 "Only assert defined vars conditionally" Author: Samuel Browne <[email protected]> Date: Thu Sep 28 13:51:05 2023 -0600 (6 days ago) (which I had suggested at the time). This new implementation sets HAVE_<PACKAGE_NAME_UC>_<UPSTREAM_PACKAGE_NAME_UC>=OFF at the project level right away when the dependencies are being read in and <upstreamPackageName> is not defined. This makes for more robust downstream CMake code that works with optional upstream dependencies when those dependencies are not defined (like when the package <upstreamPackageName> was removed from the reduced source tree by the reduced tarball feature). A downstream package should not care whether an upstream dependence is disabled or is missing entirely; the reaction should be the same and it should be able to rely on the same variables.
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.
We need to pass -DTrilinos_CONFIGURE_OPTIONS_FILE=XXX instead of -C XXX sometimes. Decided to do it for any GCC build.
User Support Ticket(s) or Story Referenced:
#12024
@trilinos/framework
Motivation
#12024