Updates for pollutant getter functionalities for links/nodes and relevant tests.

[bemason]

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.

[abhiramm7]

@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.

[abhiramm7]

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.

[bemason]

@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?

[abhiramm7]

@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.

[karosc]

I started a test repo to see if I can get things building on the latest windows and ubuntu images

[bemason]

Moved the length assignment out of the switch statement for both node and link getters.

[karosc]

Oh no, looks like the regression tests are failing on windows 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. Still experimenting though, github actions config is new to me.

[karosc]

Ok, so I got Ubuntu 20.04 to work by adding an option to statically link libraries using the CMake BUILD_SHARED_LIBS. This required very minor edits to CMakeLists.txt files as well as some modifications to the Linux make.sh file 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 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, 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, I think due to minor differences in model results from the benchmarks. 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?

[abhiramm7]

@bemason can you update your PR to the latest commit? @karosc fixed the tests.

[karosc]

@bemason, you should be able to rebase your fork on the OWA develop branch to get tests passing. However, you might think about resetting your fork back 2 commits ago to avoid merging those pesky debugging commits.

[bemason]

@karosc can you explain how I do the two steps you suggested? I am not super familiar with these processes.

[karosc]

No problem @bemason. These commands did the trick for me locally, but naturally I couldn't push to your repo. I added comments to each line to explain what each is supposed to do.

# undo the last two commits and trash the changes
$git reset HEAD~2 --hard

# show the latest commit at HEAD to make sure we did it right
$git log

commit 235beb30a216a8645e237d5062f92280a4582141 (HEAD -> develop)
Author: bemason <bemason@umich.edu>
Date:   Mon Apr 11 15:04:42 2022 -0400

    Fixed error in swmm_setLinkPollut() in toolkit.c

    Checked object index but incorrectly called Nobjects[NODE] instead of Nobjects[LINK]

commit 4f3aa95ec04c26943692d6b01f60b7f2fd3484af
Author: bemason <bemason@umich.edu>
Date:   Wed Apr 6 11:08:12 2022 -0400

    Update toolkit.c

    Moved length statement out of switch case  for both node and link getters to make the code more concise.

# add OWA SWMM repo as an upstream source
$git remote add upstream https://github.com/OpenWaterAnalytics/Stormwater-Management-Model.git

# fetch latest changes from OWA SWMM
$git fetch upstream
remote: Enumerating objects: 38, done.
remote: Counting objects: 100% (38/38), done.
remote: Compressing objects: 100% (11/11), done.
remote: Total 28 (delta 14), reused 19 (delta 9), pack-reused 0
Unpacking objects: 100% (28/28), 3.94 KiB | 269.00 KiB/s, done.
From https://github.com/OpenWaterAnalytics/Stormwater-Management-Model
 * [new branch]      actions_boost_fix  -> upstream/actions_boost_fix
 * [new branch]      attic              -> upstream/attic
 * [new branch]      dev-5.1.13         -> upstream/dev-5.1.13
 * [new branch]      dev-doxygen        -> upstream/dev-doxygen
 * [new branch]      develop            -> upstream/develop
 * [new branch]      dleutnant-patch-1  -> upstream/dleutnant-patch-1
 * [new branch]      draft_release      -> upstream/draft_release
 * [new branch]      epa-master         -> upstream/epa-master
 * [new branch]      feature-2dflood    -> upstream/feature-2dflood
 * [new branch]      feature-rainapi    -> upstream/feature-rainapi
 * [new branch]      feature-reentrancy -> upstream/feature-reentrancy
 * [new branch]      gh-pages           -> upstream/gh-pages
 * [new branch]      lrossman-dev       -> upstream/lrossman-dev
 * [new branch]      master             -> upstream/master
 * [new branch]      toolkitapi         -> upstream/toolkitapi

# add your changes to SWMM on top of latest OWA commits that your fork is missing (i.e. the ci fixes)
$git rebase upstream/develop
Successfully rebased and updated refs/heads/develop.

# push your local repo to github ignoring remote state since we deleted two commits and it would error otherwise (called a force push)
# you might want to do a quick local QA check to make sure everything builds on your end and that your changes
# are accurately reflected at this point before force pushing
$git push origin master --force

[bemason]

@karosc thank you so much for that tutorial! I followed those steps, did a local QA check, built/ran tests, and everything looks good on my end as far as I can see. Hopefully I did everything correctly?

[karosc]

Something definitely went wrong, since I'm seeing duplicate commits in you github repo. Did you rebase and force push or do something else?

[karosc]

I just force pushed the local repo I created from your fork last night to my fork and was hoping your commit history would look more like that.

Maybe I'm being picky about the commit history, but it serves as record of changes and I feel like it should be a clean as possible. If the maintainers of this repo feel otherwise, I think they should feel free to merge this PR since it does contain the proper pollutant fixes you implemented and is passing tests with the new GitHub action runners.

[bemason]

@karosc yes I followed your steps but when I ran force push it gave me an error saying it failed to push some refs. So I ended up going into Github Desktop and fixing the issues and then pushing the changes. I thought that fixed the issue but I guess it did not? I am not sure how else to rectify this now other than just deleting my branch and re-adding all my changes to the newest develop branch commit. That would then ensure the commits are all correct. There are not that many changes so it would take too long. Thoughts on this approach?

[karosc]

That makes sense, git can be headache sometimes. I think deleting your fork might automatically close this PR. But, then you could re-fork the OWA repo, re-commit your changes, and re-submit the PR with a cleaner commit history. I'd just include a reference to this PR in your new PR so the discussions here that are associated with your changes are easy to find.

[michaeltryby]

@karosc, Just want to add my two cents here ... @bemason and @abhiramm7 have been working on this feature for a long time; as a matter of fact, since before you became active as a developer here. I think the commit history on this PR is good enough. We have certainly merged messier ones in the past. Keep in mind that we have to work with developers with varying degrees of comfort and experience using tools like git. Therefore, we need to keep things simple for git newbies.

[karosc]

As I said above, if the maintainers of this repo feel comfortable with everything, I think this is ready for merging.

[michaeltryby]

@bemason and @abhiramm7 Thanks for all your hard work on this feature. Its a great addition to OWA SWMM. @abhiramm7 do the honor of merging this PR for us.

[michaeltryby]

This was a great team effort all the way around!

[abhiramm7]

awesome! merging it in 🥳