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.

Sign up for GitHub

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

Jump to bottom

Isorropia: EpetraExt tests accidentally disabled 10 years ago are enabled with updated TriBITS #10534

Closed
bartlettroscoe opened this issue May 17, 2022 · 3 comments
Closed

Isorropia: EpetraExt tests accidentally disabled 10 years ago are enabled with updated TriBITS #10534

bartlettroscoe opened this issue May 17, 2022 · 3 comments
Labels
CLOSED_DUE_TO_INACTIVITY Issue or PR has been closed by the GitHub Actions bot due to inactivity. MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. pkg: Isorropia type: bug The primary issue is a bug in Trilinos code or tests

Comments

@bartlettroscoe
Copy link
Member

bartlettroscoe commented May 17, 2022 •
edited
Loading

Bug Report

@trilinos/isorropia

Description

While testing the updated version of TriBITS, I came across a bug in the Isorropia build and test system introduced 10 years ago in the commit 7aabd5d. What happened is that EpetraExt was moved from an optional upstream to a required upstream package but if() statements based on ${PACKAGE_NAME}_ENABLE_EpetraExt were never removed. As a result, code and tests at:

Trilinos/packages/isorropia/test/CMakeLists.txt

Line 13 in 0bd35b1

IF (${PACKAGE_NAME}_ENABLE_EpetraExt)

and

Trilinos/packages/isorropia/src/CMakeLists.txt

Line 9 in 0bd35b1

SET(HAVE_EPETRAEXT ${PACKAGE_NAME}_ENABLE_EpetraExt)

did not get modified.

What is funny is that the setting of HAVE_EPETRAEXT was incorrectly set as:

Trilinos/packages/isorropia/src/CMakeLists.txt

Line 9 in 0bd35b1

SET(HAVE_EPETRAEXT ${PACKAGE_NAME}_ENABLE_EpetraExt)

instead of as:

SET(HAVE_EPETRAEXT ${${PACKAGE_NAME}_ENABLE_EpetraExt})

by me way back when Isorropia was first transitioned to CMake 13+ years ago in commit e8a6263!

However, the if statement at

Trilinos/packages/isorropia/src/CMakeLists.txt

Line 9 in 0bd35b1

SET(HAVE_EPETRAEXT ${PACKAGE_NAME}_ENABLE_EpetraExt)

was basically unconditionally disabled in commit 7aabd5d so those tests have not been enabled in 10+ years.

With the update of TriBITS to set ${PACKAGE_NAME}_ENABLE_<depPkg> to be set to TRUE for all required dependencies as well (see TriBITSPub/TriBITS@630d877 and TriBITSPub/TriBITS#479), the variable ${PACKAGE_NAME}_ENABLE_EpetraExt is getting set to TRUE with new TriBITS and test code that has not been enabled for 10+ years is getting enabled and is failing to build (see #10533).

Steps to Reproduce

See #10533
@bartlettroscoe bartlettroscoe added type: bug The primary issue is a bug in Trilinos code or tests pkg: Isorropia labels May 17, 2022
@bartlettroscoe
Copy link
Member Author

bartlettroscoe commented May 17, 2022 •
edited
Loading

What seems to have happened is that the person that did the commit 7aabd5d did not check to make sure that the same number of tests were enabled and ran.

@bartlettroscoe bartlettroscoe mentioned this issue May 17, 2022
Merged
4 tasks
bartlettroscoe added a commit to bartlettroscoe/Trilinos that referenced this issue May 17, 2022
@bartlettroscoe
Isorropria: Unconditionally disable tests that depend on EpetraExt (t…
d8965b3 
…rilinos#10534)

This test code has not been enabled or built in over 10 years due to a defect
added in commit:

  7aabd5d "Isorropia: Make EpetraExt a required dependency"
  Author: Brent Perschbacher <[email protected]>
  Date:   Tue Feb 21 13:37:59 2012 -0700 (10 years ago)

  M       packages/isorropia/cmake/Dependencies.cmake

But note that HAVE_EPETRA was getting set to true unconditionally due to a
defect in how it was set as:

  SET(HAVE_EPETRAEXT ${PACKAGE_NAME}_ENABLE_EpetraExt)

instead of as dereferencing the variable with:

  SET(HAVE_EPETRAEXT ${${PACKAGE_NAME}_ENABLE_EpetraExt})

So the value "Isorropia_ENABLE_EpetraExt" evaluates to true in CMake so
EpetraExt support was always required, even if EpetraExt support in Isorropia
was disabled!

So I just fixed this to make it clear that this was **always** on by changing
this to:

  SET(HAVE_EPETRAEXT ON)

This test code was getting switched on again with the new TriBITS that sets
Isorropia_ENABLE_EpetraExt to TRUE, even for required dependencies (see PR
@bartlettroscoe bartlettroscoe mentioned this issue May 18, 2022
Merged
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue May 27, 2022
Merge 'trilinos/Trilinos:develop' (3fa0738) into 'tcad-charon/Trilino…
ca67152 
…s:develop' (f7c3706).

* trilinos-develop:
  FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534)
  Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299)
  ATDM: Strip leading and trailing whitespace from flags for newer CMakes
jmgate pushed a commit to tcad-charon/Trilinos that referenced this issue May 27, 2022
Merge 'trilinos/Trilinos:develop' (3fa0738) into 'tcad-charon/Trilino…
3592578 
…s:develop' (f7c3706).

* trilinos-develop:
  FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534)
  Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299)
  ATDM: Strip leading and trailing whitespace from flags for newer CMakes
cgcgcg pushed a commit to cgcgcg/Trilinos that referenced this issue Jun 7, 2022
Merge 'trilinos/Trilinos:trilinos/develop' (c0f558a) into 'EM-Plasma/…
41d38d7 
…Trilinos:develop' (7d907aa).

* trilinos/develop: (31 commits)
  MiniEM: Fix lambda capture issues in evaluators
  Phalanx: fix for non-cuda spaces
  Cmake: Tpetra deprecation removal nightlies
  Phalanx: cleanup
  Phalanx: add new ctor to v_of_v
  Phalanx: another view-of-view utility
  FindTPLNetcdf.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLLAPACK.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  FindTPLBLAS.cmake: Lower-case function names (TriBITSPub/TriBITS#274)
  Isorropria: Unconditionally disable tests that depend on EpetraExt (trilinos#10534)
  Panzer MiniEM: Disable tests for 2nd order elements
  MueLu Maxwell1: Small fix
  MiniEM: Add "p coarsen schedule"
  Zoltan2: Use single value for FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Zoltan2: Properly pass in list to FAIL_REGULAR_EXPRESSION (TriBITSPub/TriBITS#464)
  Sacado: Call target_include_directories() for new TriBITS (TriBITSPub/TriBITS#299)
  Phalanx: Array of Views acceptance test.
  ATDM: Strip leading and trailing whitespace from flags for newer CMakes
  Ifpack2 Hiptmair: Add parameter "hiptmair: implicit transpose"
  Ifpack2 Hiptmair: timers
  ...
@github-actions
Copy link

This issue has had no activity for 365 days and is marked for closure. It will be closed after an additional 30 days of inactivity.
If you would like to keep this issue open please add a comment and/or remove the MARKED_FOR_CLOSURE label.
If this issue should be kept open even with no activity beyond the time limits you can add the label DO_NOT_AUTOCLOSE.
If it is ok for this issue to be closed, feel free to go ahead and close it. Please do not add any comments or change any labels or otherwise touch this issue unless your intention is to reset the inactivity counter for an additional year.

@github-actions github-actions bot added the MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. label May 20, 2023
@github-actions
Copy link

This issue was closed due to inactivity for 395 days.

@github-actions github-actions bot added the CLOSED_DUE_TO_INACTIVITY Issue or PR has been closed by the GitHub Actions bot due to inactivity. label Jun 21, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
CLOSED_DUE_TO_INACTIVITY Issue or PR has been closed by the GitHub Actions bot due to inactivity. MARKED_FOR_CLOSURE Issue or PR is marked for auto-closure by the GitHub Actions bot. pkg: Isorropia type: bug The primary issue is a bug in Trilinos code or tests
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
1 participant
@bartlettroscoe