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.

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

Fix #8722 - HVAC-Diagram.exe broken in v9.5 (Modernize CMake Fortran Settings) #8823

Merged
merged 10 commits into from
Jul 16, 2021

Conversation

jmarrec
Copy link
Contributor

@jmarrec jmarrec commented Jun 15, 2021

Pull request overview

WIP: I got it building and running fine on Windows, but didn't realize Unix broke...Edit: this should be fixed now

The problem is that on Windows it seems that CMakeAddFortranSubdirectory doesn't know about the target fortran_project_options if included via toplevel CMakeList.txt include(cmake/Fortran.cmake). On Unix it seems that each individual subdirectory is aware of it, resulting in a target name clash.

CMake Error at cmake/Fortran.cmake:1 (add_library):
  add_library cannot create target "fortran_project_options" because another
  target with the same name already exists.  The existing target is an
  interface library created in source directory
  "/home/julien/Software/Others/EnergyPlus/src/ExpandObjects".  See
  documentation for policy CMP0002 for more details.
Call Stack (most recent call first):
  src/ReadVars/CMakeLists.txt:19 (include)

Pull Request Author

Add to this list or remove from it as applicable. This is a simple templated set of guidelines.

  • Title of PR should be user-synopsis style (clearly understandable in a standalone changelog context)
  • Label the PR with at least one of: Defect, Refactoring, NewFeature, Performance, and/or DoNoPublish
  • Pull requests that impact EnergyPlus code must also include unit tests to cover enhancement or defect repair
  • Author should provide a "walkthrough" of relevant code changes using a GitHub code review comment process
  • If any diffs are expected, author must demonstrate they are justified using plots and descriptions
  • If changes fix a defect, the fix should be demonstrated in plots and descriptions
  • If any defect files are updated to a more recent version, upload new versions here or on DevSupport
  • If IDD requires transition, transition source, rules, ExpandObjects, and IDFs must be updated, and add IDDChange label
  • If structural output changes, add to output rules file and add OutputChange label
  • If adding/removing any LaTeX docs or figures, update that document's CMakeLists file dependencies

Reviewer

This will not be exhaustively relevant to every PR.

  • Perform a Code Review on GitHub
  • If branch is behind develop, merge develop and build locally to check for side effects of the merge
  • If defect, verify by running develop branch and reproducing defect, then running PR and reproducing fix
  • If feature, test running new feature, try creative ways to break it
  • CI status: all green or justified
  • Check that performance is not impacted (CI Linux results include performance check)
  • Run Unit Test(s) locally
  • Check any new function arguments for performance impacts
  • Verify IDF naming conventions and styles, memos and notes and defaults
  • If new idf included, locally check the err file and other outputs

@jmarrec jmarrec added Defect Includes code to repair a defect in EnergyPlus AuxiliaryTool Related to an auxiliary tool, not EnergyPlus itself (readvars, preprocessor, ep-launch, etc.) labels Jun 15, 2021
@jmarrec jmarrec requested a review from Myoldmopar June 15, 2021 15:09
@jmarrec jmarrec self-assigned this Jun 15, 2021
@jmarrec jmarrec removed the request for review from Myoldmopar June 15, 2021 15:12
@jmarrec jmarrec force-pushed the 8722_HVAC_Diagram branch from 13fadd1 to 92fbf39 Compare June 15, 2021 15:50
jmarrec added 2 commits June 15, 2021 18:07
…ch fortran subdirectory

On Unix it is seen and detected as multiple targets defined with the same name (CMP0002), so only do it once at top level.
I changed a `include_directory()` to a target_include_directory but forgot to do it for each Transition lib as well.
@jmarrec
Copy link
Contributor Author

jmarrec commented Jun 16, 2021

The GCC on-premise machine are failing due to a way too old cmake. target_link_options was added in cmake 3.13: https://cmake.org/cmake/help/latest/command/target_link_options.html

@jmarrec
Copy link
Contributor Author

jmarrec commented Jun 22, 2021

Gneeeeeee why you ruining my life windows?! Previous build had worked, last is failing... In between I only merged develop.
I'll investigate why windows is failing tomorrow by trying locally...

@jmarrec
Copy link
Contributor Author

jmarrec commented Jul 1, 2021

So this unrelated build has the same windows error about Transition: https://nrel.github.io/EnergyPlusBuildResults/EnergyPlus-c29a7faf1a977cf3396a8c11c50e42b34d2c9646-Win64-Windows-10-VisualStudio-16.html (coming from PR #8857)

Same problem on #8831

Copy link
Member

@Myoldmopar Myoldmopar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Who knew that in 2021, @jmarrec would become a master detective of CMake Fortran subproject quirks? This is great stuff @jmarrec, we should discuss whether this is being tested exhaustively enough to call it complete. We can definitely add some tests to the EPTravisTester engine for these auxiliary pieces, but we also want to make sure Decent is doing a decent job of it as well. From a code perspective this looks good.

add_executable(AppGPostProcess ${SRC})
set_target_properties(AppGPostProcess PROPERTIES FOLDER Auxiliary)

if(NOT UNIX) # Need to reinclude it on Windows
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is gross. And a good fix.

set(VERSIONS
1_0_0 1_0_1 1_0_2 1_0_3 1_1_0 1_1_1 1_2_0 1_2_1 1_2_2 1_2_3 1_3_0 1_4_0
2_0_0 2_1_0 2_2_0 3_0_0 3_1_0 4_0_0 5_0_0 6_0_0 7_0_0 7_1_0 7_2_0
8_0_0 8_1_0 8_2_0 8_3_0 8_4_0 8_5_0 8_6_0 8_7_0 8_8_0 8_9_0
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am compelled to want the 3's, 4, 5, 6, and 7's in their own lines....... 👀 But I won't hold this up for it.

@Myoldmopar
Copy link
Member

I confirmed the package build is good on Linux. EP-Compare now at least opens up without complaining about the GraphHints being missing. HVAC Diagram is working fine on Linux, as it was, and for Windows I am relying on the fact that you could reproduce the problem and see the fix. I ran unit tests on the local build with develop pulled in, no problem. CI is super happy with this branch. This is good to go in. Thanks @jmarrec! (As a follow-up, we could add some testing of hvac diagram into the EPTravisTester, we'll discuss later)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
AuxiliaryTool Related to an auxiliary tool, not EnergyPlus itself (readvars, preprocessor, ep-launch, etc.) Defect Includes code to repair a defect in EnergyPlus
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HVAC-Diagram.exe broken in v9.5 EP-Compare can't start (Linux)
7 participants