Repository: pyswmm/Stormwater-Management-Model (language: python)

Issue:
Updates for pollutant getter functionalities for links/nodes and relevant tests.
1. Fixed spacing for swmm_getNodePollut.
2. Added *length parameter to the SM_NODECIN and SM_NODEREACTORC cases in getNodePollut().
3. Added case for SM_LINKREACTORC in swmm_getLinkPollut().
4. Added a check to confirm length is getting pulled/set correctly for link/node pollutants. 
5. Added a test for link reactorQual.


Discussion / hints:
@bemason These changes look good to me. I believe the only thing we need to resolve is the windows 2016 test, which was what @michaeltryby was asking us to do. I am not yet sure where the test bug is popping up from, if you all have availability I would start from here https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/blob/develop/.github/workflows/build-test.yml, if not I can take stab at it. 
https://github.com/OpenWaterAnalytics/Stormwater-Management-Model/issues/340, we also seem to have an issue with the ubuntu test as well. We will have to patch this as well. 
@karosc I can move the length statement out of the switch statements tomorrow. @abhiramm7 I am really unsure how to work on the windows 2016 and ubuntu test? I don't even know what you mean by having to "patch" it.  Would you mind taking a stab at it? 
@bemason yeah for sure, I'll dig into it. I was trying to sound like a fancy developer 😅. We have to update the yml file to make sure the ubuntu and windows tests run. 
I started a test repo to see if I can get things building on the latest windows and ubuntu images
Moved the length assignment out of the switch statement for both node and link getters.
Oh no, looks like the regression tests are [failing on windows](https://github.com/karosc/testSWMM/runs/5854297677?check_suite_focus=true#step:10:108) after updating to Visual Studio 2022.

Still have not figured out how to fix ubuntu symbol lookup error after updating to 20.04. Responding to @michaeltryby's thoughts in #340, I am wondering if we can fix [RPATH issues using cmake](https://gitlab.kitware.com/cmake/community/-/wikis/doc/cmake/RPATH-handling). Still experimenting though,  github actions config is new to me.
Ok, so I got Ubuntu 20.04 to work by adding an option to statically link libraries using the CMake [BUILD_SHARED_LIBS](https://cmake.org/cmake/help/latest/guide/tutorial/Selecting%20Static%20or%20Shared%20Libraries.html). This required very minor edits to CMakeLists.txt files as well as some modifications to the [Linux make.sh file](https://github.com/karosc/ci-tools/commit/888212968e12ca4ce9821fce1900e4b5d2f6dea7) so tests would link static instead of shared. With my changes if you run `make.sh -s -t -g "Unix Makefiles"` the -s flag will make libswmm5 and libswmm-output static libraries instead of shared.

The working runners are in a [test repo I setup](https://github.com/karosc/SWMM_GH_Actions_Tester) so I wouldn't pollute the commit history with my tiny edits for learning/debugging github actions. This new repo points to [my fork of the ci-tools](https://github.com/karosc/ci-tools), so I think we would need PRs for both ci-tools as well as SWMM.

Even though the actions are running now, the [regression tests are failing](https://github.com/karosc/SWMM_GH_Actions_Tester/actions/runs/2106069361), I think due to minor differences in model results from [the benchmarks](https://github.com/OpenWaterAnalytics/swmm-nrtestsuite). I suspect this comes with the territory of updating a compiler to a new version, no? Perhaps @michaeltryby or @cbuahin could comment since they seem to be the primary authors of the ci-tools?

Here are some of my questions moving forward:

1. Does it make sense to update all the regression testing benchmarks to accommodate these updates? Or am I doing something else incorrectly that is causing the failures?
2. What PR order makes the most sense. From my perspective it should be ci-tools, SWMM, then swmm-nrtestsuite.
3. Should updating the github actions in SWMM be a separate pull request from this one? If so, what order makes sense? Should we get the new action runners working and passing tests before merging in these pollutant updates?

Fix patch:
diff --git a/CMakeLists.txt b/CMakeLists.txt
index d3255fbac..087455567 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -49,6 +49,8 @@ set(CONFIG_DIST  "cmake")
 option(BUILD_TESTS "Build component tests (requires Boost)" OFF)
 option(BUILD_DOCS "Build toolkit docs (requires Doxygen)" OFF)
 
+# Added option to statically link libraries to address GitHub Ubuntu 20.04 symbol errors (issue #340)
+option(BUILD_SHARED_LIBS "Build using shared libraries" ON)
 
 # Add project subdirectories
 if(BUILD_DOCS)
diff --git a/src/outfile/CMakeLists.txt b/src/outfile/CMakeLists.txt
index 9b44c476c..11b7e5ce8 100644
--- a/src/outfile/CMakeLists.txt
+++ b/src/outfile/CMakeLists.txt
@@ -18,8 +18,7 @@ set(SWMM_OUT_PUBLIC_HEADERS
 
 
 # the binary output file API
-add_library(swmm-output
-    SHARED
+add_library(swmm-output    
         swmm_output.c
         errormanager.c
 )
diff --git a/src/solver/CMakeLists.txt b/src/solver/CMakeLists.txt
index db0d8636f..e10fa4a9a 100644
--- a/src/solver/CMakeLists.txt
+++ b/src/solver/CMakeLists.txt
@@ -38,8 +38,7 @@ if(BUILD_DEF)
         PROPERTIES_HEADER_FILE_ONLY TRUE
     )
 
-    add_library(swmm5
-        SHARED
+    add_library(swmm5        
             ${SWMM_SOURCES}
             ${PROJECT_SOURCE_DIR/bindings/swmm5.def}
             $<TARGET_OBJECTS:shared_objs>
@@ -47,8 +46,7 @@ if(BUILD_DEF)
 
 else()
     # Performs standard library build
-    add_library(swmm5
-        SHARED
+    add_library(swmm5        
             ${SWMM_SOURCES}
             $<TARGET_OBJECTS:shared_objs>
     )


Test patch:
diff --git a/.github/workflows/build-test.yml b/.github/workflows/build-test.yml
index c9c5b5554..41e7c5020 100644
--- a/.github/workflows/build-test.yml
+++ b/.github/workflows/build-test.yml
@@ -20,15 +20,15 @@ jobs:
     strategy:
       fail-fast: false
       matrix:
-        os: [windows-2016, ubuntu-16.04, macos-10.15]
+        os: [windows-2022, ubuntu-20.04, macos-10.15]
         requirements: [requirements-swmm.txt]
         include:
-          - os: windows-2016
+          - os: windows-2022
             sys_pkgs: choco install boost-msvc-14.1
-            build_unit_test: make.cmd -t -g "Visual Studio 15 2017 Win64"
-            build_reg_test:  make.cmd -g "Visual Studio 15 2017 Win64"
+            build_unit_test: make.cmd /g "Visual Studio 17 2022" /A "x64" /t
+            build_reg_test: make.cmd /g "Visual Studio 17 2022" /A "x64"
             before_reg_test: before-nrtest.cmd
-            run_reg_test:    run-nrtests.cmd
+            run_reg_test: run-nrtests.cmd
             build_id: "%GITHUB_RUN_ID%_%GITHUB_RUN_NUMBER%"
             experimental: false
             artifacts_ext: zip
@@ -38,12 +38,13 @@ jobs:
                 shell: cmd
                 working-directory: ./ci-tools/windows
 
-          - os: ubuntu-16.04
+          - os: ubuntu-20.04
             sys_pkgs: sudo apt install libboost-dev libboost-all-dev
-            build_unit_test: make.sh -t -g "Unix Makefiles"
-            build_reg_test:  make.sh -g "Unix Makefiles"
+            # Statically link libraries with -s switch to address GitHub Ubuntu 20.04 symbol errors (issue #340)
+            build_unit_test: make.sh -s -t -g "Unix Makefiles"
+            build_reg_test: make.sh -s -g "Unix Makefiles"
             before_reg_test: before-nrtest.sh
-            run_reg_test:    run-nrtests.sh
+            run_reg_test: run-nrtests.sh
             build_id: ${GITHUB_RUN_ID}_${GITHUB_RUN_NUMBER}
             experimental: true
             artifacts_ext: tar.gz
@@ -56,9 +57,9 @@ jobs:
           - os: macos-10.15
             sys_pkgs: brew install libomp boost
             build_unit_test: make.zsh -t -g "Xcode"
-            build_reg_test:  make.zsh -g "Xcode"
+            build_reg_test: make.zsh -g "Xcode"
             before_reg_test: before-nrtest.zsh
-            run_reg_test:    run-nrtests.zsh
+            run_reg_test: run-nrtests.zsh
             build_id: ${GITHUB_RUN_ID}_${GITHUB_RUN_NUMBER}
             experimental: true
             artifacts_ext: tar.gz
@@ -80,7 +81,6 @@ jobs:
       BUILD_HOME: build
       TEST_HOME: nrtests
 
-
     steps:
       - name: Checkout repo
         uses: actions/checkout@v2
@@ -114,7 +114,7 @@ jobs:
       - name: Before reg test
         env:
           NRTESTS_URL: https://github.com/OpenWaterAnalytics/swmm-nrtestsuite
-          BENCHMARK_TAG: v1.0.4-dev
+          BENCHMARK_TAG: v2.0.1
         run: ./${{ matrix.before_reg_test }} ${{ env.BENCHMARK_TAG }}
 
       - name: Run reg test