Cmake modernize #8425

lefticus · 2020-12-15T15:23:28Z

Pull request overview

Modernize CMake

prefer target_ directives over global settings
use cmake-format for consistent formatting
add a .cmake-format.yaml file at the top level which can be adjusted as needed

cmake-format is a pip package, easily installable on all platforms.

* Reduce global settings

* uses project_warnings and project_options interface projects * now we no longer force our warning flags on third_party projects

* found by enabling more warnings on clang * previous code was invoking undefined behavior by deleting an object via a pointer to its base class with a non-virtual destructor * Could have caused memory leaks (or worse)

This reverts commit b059d34.

…nize

Myoldmopar · 2020-12-29T19:43:31Z

@lefticus so something is happening on this PR. We noticed Windows CI was struggling a couple times and eventually deduced that this branch was just eating days of the CI time. Just on Windows. You can see the results from that last commit and how the test time was about 220843 seconds. I'm not sure what changed on the build on Windows, but something definitely went awry.

For reference, the bigobj change already dropped into develop this morning and hasn't caused any issues with CI, so it's not that.

lefticus · 2020-12-30T01:37:57Z

I just did a review of the code, building both Release and Debug locally, unable to reproduce any issues. I know earlier versions of this completed in a reasonable time on Windows, maybe it was a fluke? I just did a merge of develop and a new push to CI. Hopefully it won't take things down again. I'll keep investigating.

…

-Jason

On Tue, Dec 29, 2020 at 12:43 PM Edwin Lee ***@***.***> wrote: @lefticus <https://github.com/lefticus> so something is happening on this PR. We noticed Windows CI was struggling a couple times and eventually deduced that this branch was just eating days of the CI time. Just on Windows. You can see the results <https://nrel.github.io/EnergyPlusBuildResults/EnergyPlus-fde2e6503d8059499dcca76068b6d57e6131bae3-Win64-Windows-10-VisualStudio-16.html> from that last commit and how the test time was about 220843 seconds. I'm not sure what changed on the build on Windows, but something definitely went awry. For reference, the bigobj change already dropped into develop this morning and hasn't caused any issues with CI, so it's not that. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8425 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZGJ56XRAD2G6NHD34MM3SXIWPBANCNFSM4U4P674A> .

-- C++ Trainer, Developer and Speaker Check out my training options: https://emptycrate.com/training.html Subscribe to learn about new training events: https://www.subscribepage.com/emptycrate

Myoldmopar · 2020-12-30T21:32:34Z

It seems to be happening again. I logged into the CI box and found these error messages popping up. Again, only on this branch.

lefticus · 2020-12-30T23:16:59Z

Ah, thank you, that gives me a good clue. Not sure how far I can get with that before the end of this year/contract. (Of course It Works For Me)

…

On Wed, Dec 30, 2020 at 2:32 PM Edwin Lee ***@***.***> wrote: It seems to be happening again. I logged into the CI box and found these error messages popping up. Again, only on this branch. [image: Screen Shot 2020-12-30 at 9 11 02 AM] <https://user-images.githubusercontent.com/849824/103382305-2fb6a600-4ab4-11eb-8059-061157f37489.png> [image: Screen Shot 2020-12-30 at 9 10 45 AM] <https://user-images.githubusercontent.com/849824/103382309-33e2c380-4ab4-11eb-9d0c-897297fda878.png> — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#8425 (comment)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABZGJ7BU2M52ZCVIFGJNSDSXOL77ANCNFSM4U4P674A> .

-- C++ Trainer, Developer and Speaker Check out my training options: https://emptycrate.com/training.html Subscribe to learn about new training events: https://www.subscribepage.com/emptycrate

This reverts commit 30b8df1.

Myoldmopar · 2021-01-03T03:42:24Z

@lefticus OK, that reversion definitely took care of the runtime issue. With the last build everything reported back in expected fashion.

There were a few build warnings on the last build though:

Mac is reporting a few unused variables in our own code which I can easily address.
Linux is reporting some build warnings in a third-party folder. I'd need to look at your modernized code to determine the best practice for muting things now.
Windows is reporting a bunch of warnings in the E+ code. I think we may need to itemize them to determine how to proceed with them. As an aside, it looks like the build warnings aren't being parsed properly by Decent CI as the links from that page aren't working. Some line/column number processing is off and it isn't doing some path manipulation right either. I'm not sure if something you modified would've changed the form of the Visual Studio error message, but I'll just need to figure out the pattern and fix it over in our fork of Decent.

It would be good to get some more feedback from others if they are interested.

Myoldmopar

I still need to look a little further, but overall, this is such a nice standardization of our cmake rules. And we have so, so many.

Myoldmopar · 2021-01-03T03:44:07Z

testfiles/CMakeLists.txt

+add_simulation_test(IDF_FILE CoolingTower_VariableSpeed_CondEntTempReset.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
+add_simulation_test(IDF_FILE CoolingTower_VariableSpeed_CondEntTempReset_MultipleTowers.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
+add_simulation_test(IDF_FILE CoolingTower_VariableSpeed_IdealCondEntTempSetpoint.idf EPW_FILE USA_IL_Chicago-OHare.Intl.AP.725300_TMY3.epw)
+add_simulation_test(IDF_FILE CoolingTower_VariableSpeed_IdealCondEntTempSetpoint_MultipleTowers.idf EPW_FILE


Any change to this file may require some polishing in a couple places:

The regression tool may still look to this file for matching EPWs with IDFs. I actually can't recall at this very moment.

There is a custom_check script that verifies all IDFs found in the testfiles directory are listed in this file. I think it does a simple line-by-line match, so line breaking will cause potential false positive/negatives and what-not.

Myoldmopar · 2021-01-03T03:44:22Z

.cmake-format.yaml

@@ -0,0 +1,229 @@
+_help_parse: Options affecting listfile parsing


Standardized format. Nice. I wonder what all editors respect this file.

Myoldmopar · 2021-01-03T03:46:17Z

CMakeLists.txt

 endif()
+target_include_directories(project_options INTERFACE ${PROJECT_SOURCE_DIR}/third_party/kiva-ep/src)
+target_include_directories(project_options INTERFACE "${kiva_BINARY_DIR}/src/libkiva")
+target_include_directories(project_options SYSTEM INTERFACE "${kiva_SOURCE_DIR}/vendor/boost-1.61.0/")


Did these have to get pulled from the third_party directory rules?

Myoldmopar · 2021-01-03T03:47:49Z

cmake/CompilerFlags.cmake

+  #    ADD_CXX_DEFINITIONS("-d2SSAOptimizer-") # this disables this optimizer which has known major issues
+
+  # ADDITIONAL RELEASE-MODE-SPECIFIC FLAGS
+  target_compile_options(project_options INTERFACE $<$<CONFIG:Release>:/GS->) # Disable buffer overrun checks for performance in release mode


nrel-bot-3 · 2021-01-31T00:31:31Z

@lefticus @lgentile it has been 28 days since this pull request was last updated.

lefticus · 2021-02-10T21:14:38Z

@Myoldmopar this line of code, by using abs is actually doing an integral comparison (this is caught now because we have more warnings enabled)

https://github.com/NREL/EnergyPlus/blob/cmake_modernize/src/EnergyPlus/Coils/CoilCoolingDXCurveFitSpeed.cc#L600

This is almost certainly not the intention. It can be fixed by moving to std::abs (the C++ version, which has overloads), but I wanted some input and sanity check before I made this change.

…nize

Myoldmopar · 2021-02-11T17:44:21Z

Yay! Custom check is all clean. I know you still need to pull develop in, merge any conflicts, and mark it ready to review, so I'll be ready to review and merge once it's all good to go. Thanks @lefticus !

…nize

Myoldmopar · 2021-02-12T14:09:06Z

All green here! Thanks @lefticus

lefticus added 13 commits December 8, 2020 07:47

Initial .cmake-format file

6378fb9

Apply cmake-format across non-third-party projects

dbc15da

Start moderizing through the use of project_options

d1a7af0

* Reduce global settings

CompilerFlags modernization

9abbe80

* uses project_warnings and project_options interface projects * now we no longer force our warning flags on third_party projects

Add missing sanitizers.cmake file

466a544

Fix some include path settings

12f191a

More cmake modernization updates

d7ffe27

Begin organization of Fortran CMake project modernization

67f039d

Start to modernize Fortran CMake

30b8df1

Reorganize Fortran CMake settings

b059d34

Add missing files

d742df6

Modernize SQLite/CMakeLists.txt

6b1fd63

Merge branch 'develop' into cmake_modernize

9b3c4b7

lefticus added the Refactoring Includes code changes that don't change the functionality of the program, just perform refactoring label Dec 15, 2020

lefticus added 14 commits December 15, 2020 15:34

MSVC build fixes

7af593f

Add /bigobj for Windows build

9a874a6

Separate out FP flags for build options

b44b787

Ensure that gtest packaging/install is disabled

38ddd74

Move header includes to after third_party is procesed

8cd400a

Make sure "enable_testing()" is executed at the top scope

46e6d43

MSVC Warning adjustments

56ce95e

Reduce warnings on GCC

50ca724

Fix non-virtual destructor issue

cd9d9a3

* found by enabling more warnings on clang * previous code was invoking undefined behavior by deleting an object via a pointer to its base class with a non-virtual destructor * Could have caused memory leaks (or worse)

Revert "Reorganize Fortran CMake settings"

26b0c38

This reverts commit b059d34.

Remove unused warning on MSVC

47d3328

Address some warnings on Windows with more warnings enabled

f0de393

Add -Wno-unused-parameters for GCC on Test API

9407ee0

Merge branch 'develop' of github.com:NREL/EnergyPlus into cmake_moder…

fde2e65

…nize

mitchute mentioned this pull request Dec 24, 2020

Global OutputReportTabular #8432

Merged

20 tasks

Merge remote-tracking branch 'origin/develop' into cmake_modernize

54fc130

lefticus added 2 commits December 30, 2020 18:59

Revert "Start to modernize Fortran CMake"

1579be0

This reverts commit 30b8df1.

Revert all Fortran CMake changes

0354a59

Myoldmopar reviewed Jan 3, 2021

View reviewed changes

Myoldmopar mentioned this pull request Feb 7, 2021

Upgrade to C++17 std::filesystem library #8480

Merged

20 tasks

lefticus added 4 commits February 9, 2021 10:17

Merge branch 'develop' into cmake_modernize

71e6c28

modernize cmake for convertinputformat

924c138

Address warnings on Windows with new more robust warning flags

8b1935b

Address warnings from GCC

e413ed4

lefticus and others added 3 commits February 10, 2021 14:16

Merge branch 'develop' of github.com:NREL/EnergyPlus into cmake_moder…

86d0b6a

…nize

Fix a few warnings found with clang-8

efb9fb4

Fix IDF verification script and cleanup testfiles/CMakeLists.txt entries

5acc1f2

lefticus added 2 commits February 11, 2021 12:03

Merge branch 'develop' of github.com:NREL/EnergyPlus into cmake_moder…

2101734

…nize

Saner line length/wrapping for cmake-format, applied across system

fe37948

lefticus marked this pull request as ready for review February 11, 2021 23:39

Myoldmopar merged commit e8bf5ea into develop Feb 12, 2021

Myoldmopar deleted the cmake_modernize branch February 12, 2021 14:09

jmarrec mentioned this pull request May 19, 2021

HVAC-Diagram.exe broken in v9.5 #8722

Closed

3 tasks

jmarrec mentioned this pull request Jun 15, 2021

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

Merged

20 tasks

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Cmake modernize #8425

Cmake modernize #8425

lefticus commented Dec 15, 2020 •

edited

Loading

Myoldmopar commented Dec 29, 2020

lefticus commented Dec 30, 2020 via email

Myoldmopar commented Dec 30, 2020

lefticus commented Dec 30, 2020 via email

Myoldmopar commented Jan 3, 2021

Myoldmopar left a comment

Myoldmopar Jan 3, 2021

Myoldmopar Jan 3, 2021

Myoldmopar Jan 3, 2021

Myoldmopar Jan 3, 2021

nrel-bot-3 commented Jan 31, 2021

lefticus commented Feb 10, 2021

Myoldmopar commented Feb 11, 2021

Myoldmopar commented Feb 12, 2021

		@@ -0,0 +1,229 @@
		_help_parse: Options affecting listfile parsing

Cmake modernize #8425

Cmake modernize #8425

Conversation

lefticus commented Dec 15, 2020 • edited Loading

Pull request overview

Myoldmopar commented Dec 29, 2020

lefticus commented Dec 30, 2020 via email

Myoldmopar commented Dec 30, 2020

lefticus commented Dec 30, 2020 via email

Myoldmopar commented Jan 3, 2021

Myoldmopar left a comment

Choose a reason for hiding this comment

Myoldmopar Jan 3, 2021

Choose a reason for hiding this comment

Myoldmopar Jan 3, 2021

Choose a reason for hiding this comment

Myoldmopar Jan 3, 2021

Choose a reason for hiding this comment

Myoldmopar Jan 3, 2021

Choose a reason for hiding this comment

nrel-bot-3 commented Jan 31, 2021

lefticus commented Feb 10, 2021

Myoldmopar commented Feb 11, 2021

Myoldmopar commented Feb 12, 2021

lefticus commented Dec 15, 2020 •

edited

Loading