Fix when Compiling with non-standard CMake Configurations #8378

mitchute · 2020-11-12T22:48:00Z

Pull request overview

Fixes when compiling with the following
- EP_nocache_Psychrometrics
- EP_cache_GlycolSpecificHeat
- LINK_WITH_PYTHON=OFF
Adds CMake commands and CI runs to build the following:
- LINK_WITH_PYTHON=OFF
- USE_PSYCHROMETRICS_CACHING=OFF
- USE_GLYCOL_CACHING=OFF
- USE_PSYCH_STATS=ON
- USE_PSYCH_ERRORS=OFF

Pull Request Author

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

Reviewer

This will not be exhaustively relevant to every PR.

…lds to test LINK_WITH_PYTHON=ON/OFF and USE_PSYCHROMETRICS_CACHING=ON/OFF

JasonGlazer · 2020-11-13T17:36:50Z

@mitchute I posted #8377 which would help catch this kind of thing in the future.

mitchute · 2020-11-13T17:41:10Z

Thanks, @JasonGlazer. This PR is adding CI runs to the GitHub actions workflows to build with LINK_WITH_PYTHON=ON/OFF and USE_PSYCHROMETRIC_CACHING=ON/OFF, so this should address #8377. Are there other macros like this we need to catch?

JasonGlazer · 2020-11-13T17:42:30Z

I don't know, I presume so but I'm not sure.

mitchute · 2020-11-13T20:31:23Z

Here are some other macros in EnergyPlus that we may want to also consider:

EP_Detailed_Timings
EP_Count_Calls
SKYLINE_MATRIX_REMOVE_ZERO_COLUMNS
EP_cache_GlycolSpecificHeat
GROUND_PLOT
EP_psych_errors
EP_psych_stats
EP_NO_OPENGL

Myoldmopar · 2020-11-14T14:34:03Z

@mitchute looks like there is indeed still some issue here, but this will be a really great addition. I don't know exactly how many of those definition conditionals we want to check...our list of possible builds could become monstrous. We surely don't need to build special versions of these on every single commit:

EP_Detailed_Timings
EP_Count_Calls
SKYLINE_MATRIX_REMOVE_ZERO_COLUMNS
GROUND_PLOT

I'd be interested in what happens if we toggle the other 4.

We could consider adding some special builds of more obscure build configurations to each tag rather than each commit. That way CI will only do those builds when we do package building.

mitchute · 2020-11-14T17:28:47Z

OK, I'm not sure what the problem is currently. I haven't seen any build problems locally on Mac and Linux, but I'll investigate more.

jasondegraw · 2020-11-16T20:32:44Z

@Myoldmopar @mitchute I'm honestly not sure if you're trolling me now, but please remove SKYLINE_MATRIX_REMOVE_ZERO_COLUMNS from the list. It isn't the flag you're looking for.

…us into fix_nocache_psychrometrics

mitchute · 2020-11-16T23:28:23Z

I'm honestly not sure if you're trolling me now

Wouldn't that be fun...

I just straight up did a search of the code for all of the preprocessor macros and listed them. No trolling intended. 😄

mitchute · 2020-11-18T16:03:38Z

@Myoldmopar This is looking like NREL CI may need to run CMake again to catch these new flags. If you take a look at the GitHub builds for USE_PSYCHROMETRICS_CACHING=OFF, you will see that they are failing with the same errors as the NREL CI machines which should have caching turned on.

…us into fix_nocache_psychrometrics

…rmating and warnings cleanup

mitchute · 2020-11-23T12:52:45Z

Currently, we have the following build matrix enabled on this branch.

CI Platform	Build Name	LINK_WITH_PYTHON	USE_PSYCHROMETRICS_CACHING	USE_GLYCOL_CACHING	USE_PSYCH_STATS	USE_PSYCH_ERRORS	OPENGL_REQUIRED
Decent CI	N/A (Every push/tag)	X	X	X		X	X
GH Actions	python_no_psychcach	X		X		X	X
GH Actions	no_python_psychcach		X	X		X	X
GH Actions	no_python_no_psychcach			X		X	X
GH Actions	no_glcol_cache		X			X	X
GH Actions	no_opengl		X	X		X
GH Actions	use_psych_stats		X	X	X	X	X
GH Actions	no_use_psych_errors		X	X	X		X

I'm not sure if we want to run them all on every push, but this should at least demonstrate that they are now all working.

mitchute · 2020-11-24T03:19:13Z

@Myoldmopar @mjwitte this is ready for review.

mjwitte

@mitchute The c++ changes all look ok to me. I'll yield to @Myoldmopar regarding the cmake and actions changes.

mjwitte · 2020-11-24T21:40:47Z

src/EnergyPlus/Psychrometrics.hh

@@ -1343,4 +1376,4 @@ namespace Psychrometrics {

 } // namespace EnergyPlus

-#endif
+#endif


This file needs an endline at the end.

I'll probably just add this to one of my upcoming globals branches and not spend more CI cycles on this.

mjwitte · 2020-11-24T21:47:50Z

src/EnergyPlus/Psychrometrics.hh

+                             [[maybe_unused]] std::string const &CalledFrom = blank_string, // routine this function was called from (error messages)
+                             [[maybe_unused]] bool const SuppressWarnings = false           // if calling function is calculating an intermediate state


I was going to question adding maybe_unused to these arguments when they have defaults, but I see now that certain arguments do truly become unused based on the state of the ifdefs. That makes sense.

Myoldmopar

No problems here, this will definitely keep Github Action instances busy for us 👍

Myoldmopar · 2020-11-25T18:52:55Z

.github/workflows/mac_build.yml

+      - name: Build EnergyPlus
+        working-directory: ${{runner.workspace}}/EnergyPlus/build
+        shell: bash
+        run: cmake --build . --target energyplus -j 2


This is fantastic!

Myoldmopar · 2020-11-25T18:53:28Z

CMakeLists.txt

+mark_as_advanced(USE_PSYCHROMETRICS_CACHING)
+mark_as_advanced(USE_GLYCOL_CACHING)
+mark_as_advanced(USE_PSYCH_STATS)
+mark_as_advanced(USE_PSYCH_ERRORS)


It's really great having some new CMake (advanced) variables for turning compiler settings on and off.

Myoldmopar · 2020-11-25T18:54:13Z

src/EnergyPlus/PluginManager.cc

@@ -1328,7 +1328,7 @@ namespace PluginManagement {
        }
    }
 #else
-    void PluginManager::setGlobalVariableValue([[maybe_unused]] EnergyPlusData &state, int EP_UNUSED(handle), Real64 EP_UNUSED(value))
+    void PluginManager::setGlobalVariableValue([[maybe_unused]] EnergyPlusData &state, [[maybe_unused]] int handle, [[maybe_unused]] Real64 value)


Yay for fixed syntax errors!

Myoldmopar · 2020-11-25T18:54:40Z

src/EnergyPlus/Psychrometrics.hh

+    constexpr int iPsyRhFnTdbRhovLBnd0C = 13;
+    constexpr int iPsyTwbFnTdbWPb_cache = 18;
+    constexpr int iPsyPsatFnTemp_cache = 19;
+    constexpr int NumPsychMonitors = 19; // Parameterization of Number of psychrometric routines that


Myoldmopar · 2020-11-25T18:54:55Z

src/EnergyPlus/Psychrometrics.hh

 #endif
 #ifdef EP_cache_PsyTsatFnHPb
-    extern Array1D<cached_tsat_h_pb> cached_Tsat_HPb; // DIMENSION(0:tsat_hbp_cache_size)
+    inline Array1D<cached_tsat_h_pb> cached_Tsat_HPb; // DIMENSION(0:tsat_hbp_cache_size)


Myoldmopar · 2020-11-30T14:38:18Z

I thought I had merged this...well I am now. Thanks so much for this @mitchute, this will make sure all these conditional code paths are building all the time now!

fix ep_nocache_psychrometrics

6125dd0

mitchute added Defect Includes code to repair a defect in EnergyPlus DoNotPublish Includes changes that shouldn't be reported in the changelog labels Nov 12, 2020

mitchute added this to the EnergyPlus Future milestone Nov 12, 2020

mitchute self-assigned this Nov 12, 2020

mitchute changed the title ~~Fix EP_nocache_Psychrometrics~~ Fix when Compiling with EP_nocache_Psychrometrics Nov 12, 2020

mitchute added 3 commits November 13, 2020 09:18

Merge branch 'develop' into fix_nocache_psychrometrics

cbd59a6

add CMake to expose EP_nocache_Psychrometrics, add github actions bui…

06c6420

…lds to test LINK_WITH_PYTHON=ON/OFF and USE_PSYCHROMETRICS_CACHING=ON/OFF

fix spelling, minor cleanups

cc00686

fix CMake for USE_PSYCHROMETRICS_CACHING

7ae4b99

mitchute added 3 commits November 16, 2020 16:23

move from 'extern T const' to 'constexpr T' in Psychrometrics

535426b

Merge branch 'fix_nocache_psychrometrics' of github.com:NREL/EnergyPl…

54dea14

…us into fix_nocache_psychrometrics

Merge branch 'develop' into fix_nocache_psychrometrics

6bf662e

mitchute added 7 commits November 16, 2020 17:02

fix link_with_python=OFF

0d717cd

test inline-ing cached_Tsat

9cdf1b6

move to inline

5d7c8a7

unify undef's

dcc84f7

build energyplus_tests on GitHub

c4eecff

build energyplus_tests on GitHub

db33906

set BUILD_TESTING=ON

abff040

Merge branch 'develop' into fix_nocache_psychrometrics

9924a3f

mitchute added 7 commits November 19, 2020 11:21

turn off build_testing on github, move CMake option up

3456a4d

Merge branch 'fix_nocache_psychrometrics' of github.com:NREL/EnergyPl…

04a4e32

…us into fix_nocache_psychrometrics

add USE_GLYCOL_CACHE CMake option and build with GH actions

398c23f

Merge branch 'develop' into fix_nocache_psychrometrics

bebdc79

OPENGL_REQUIRED=OFF to GH actions builds

ad5a9bc

add USE_PSYCH_CACHE CMake option, fix build, add to GH actions builds

5571721

move EP_psych_errors to CMake options, add GH actions build, minor fo…

3f8f345

…rmating and warnings cleanup

fix Mac unittests build

955c837

mitchute changed the title ~~Fix when Compiling with EP_nocache_Psychrometrics~~ Fix when Compiling with non-standard CMake Configurations Nov 23, 2020

add USE_PSYCH_ERRORS CMake to unittest builds

0be9c78

mjwitte approved these changes Nov 24, 2020

View reviewed changes

Myoldmopar approved these changes Nov 25, 2020

View reviewed changes

Myoldmopar merged commit c1905e6 into develop Nov 30, 2020

Myoldmopar deleted the fix_nocache_psychrometrics branch November 30, 2020 14:38

jmarrec mentioned this pull request Dec 11, 2020

building project "energyplusapi.vcxproj" -- FAILED #8418

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix when Compiling with non-standard CMake Configurations #8378

Fix when Compiling with non-standard CMake Configurations #8378

mitchute commented Nov 12, 2020 •

edited

Loading

JasonGlazer commented Nov 13, 2020

mitchute commented Nov 13, 2020

JasonGlazer commented Nov 13, 2020

mitchute commented Nov 13, 2020

Myoldmopar commented Nov 14, 2020

mitchute commented Nov 14, 2020

jasondegraw commented Nov 16, 2020

mitchute commented Nov 16, 2020

mitchute commented Nov 18, 2020

mitchute commented Nov 23, 2020

mitchute commented Nov 24, 2020

mjwitte left a comment

mjwitte Nov 24, 2020

mitchute Nov 25, 2020

Myoldmopar Nov 25, 2020

mjwitte Nov 24, 2020

Myoldmopar left a comment

Myoldmopar Nov 25, 2020

Myoldmopar Nov 25, 2020

Myoldmopar Nov 25, 2020

Myoldmopar Nov 25, 2020

Myoldmopar Nov 25, 2020

Myoldmopar commented Nov 30, 2020

		[[maybe_unused]] std::string const &CalledFrom = blank_string, // routine this function was called from (error messages)
		[[maybe_unused]] bool const SuppressWarnings = false // if calling function is calculating an intermediate state

Fix when Compiling with non-standard CMake Configurations #8378

Fix when Compiling with non-standard CMake Configurations #8378

Conversation

mitchute commented Nov 12, 2020 • edited Loading

Pull request overview

Pull Request Author

Reviewer

JasonGlazer commented Nov 13, 2020

mitchute commented Nov 13, 2020

JasonGlazer commented Nov 13, 2020

mitchute commented Nov 13, 2020

Myoldmopar commented Nov 14, 2020

mitchute commented Nov 14, 2020

jasondegraw commented Nov 16, 2020

mitchute commented Nov 16, 2020

mitchute commented Nov 18, 2020

mitchute commented Nov 23, 2020

mitchute commented Nov 24, 2020

mjwitte left a 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

Myoldmopar left a 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

Choose a reason for hiding this comment

Myoldmopar commented Nov 30, 2020

mitchute commented Nov 12, 2020 •

edited

Loading