-
Notifications
You must be signed in to change notification settings - Fork 92
Add instructions on compiling with dynamic lib + fix floating point error #143
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
base: main
Are you sure you want to change the base?
Conversation
|
Note: I think the failed test is probably related to #144 and not to the changes in this pull request. EDIT: We could probably test multiple commits (going back) to narrow where the output changes occur. I tried to do that for 61.0.1, but I ran into more floating errors, that are probably fixed now, so I did not want to go over fixing them to see if I get the same error. From my test, it is at least before the 61.0.2, I ran into compilation errors while trying to compile some commits before that (and the git tree looks really convoluted), so I did not continue. |
There will be a new PR to be more OS specific on static library creation, having flags for Arch, Debian, RH, etc. to take care of gnu OS variants and library dependencies. This will make it obsolete to modify |
odav
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
changing the type to real(8) will only mask the problem. A better solution is to replace Exp() with exp_w() from utils.90 as shown in other source files.
|
Thank you for the review. I have replaced the use of As for the Let me know if I need to change anything else. EDIT: I have merged the latest changes and force pushed it so that there is a clean history. |
|
Atreyagaurav, I was having to comment out those same lines in the CMakeList.txt to compile and link with gfortran because I use an Arch based linux distribution that does not have the necessary libraries to statically link gfortran code. The solution that Olaf and I worked on tests whether the type of linux distribution is Arch, or Fedora RH based, and if so, it doesn't try to statically link when using these distributions. Could you share with us what linux distribution you are running and test the new CMakeList.txt when it gets pushed up to swatplus main? On the Real(8) issue. Gfortran does not set to zero when the exponent in the exp function goes for too negative. IFX does this (and it shouldn't). Gfortran is actually correct in flagging this as a runtime issue. The solution for now is use w_exp function in utils.f90. You will need to add the statement 'use utils' to the subroutine nut_soilp.f90 to access w_exp function. I was aware of this particular issue in nut_solp.f90 but was using that runtime exception to help with help me figure out how the windows and linux debuggers handled that kind of error. It is time to actually fix the code. |
|
Hi @fgeter , thank you for the reviews. I use Arch Linux, I can test it when the new solution is pushed, no problem. I have already modified the code to use exp_w from utils, please refer to the git diff tab. I rebased it to keep the git commits clean. |
|
Very good. the new CMakeList.txt should work fine for you then since you are using Arch. What we are thinking is all Arch based distributions should behave in the same way. I use EndeavourOS which is based on Arch. Arch based distributions cannot do static linking. I think also that all debian based distributions should behave the same. That includes Ubuntu, Linux Mint and host others. Debian based distributions should be able link statically. Fedora based distribution also cannot do static linking. A big issue is we cannot test every distribution but I think we a way have way now to fix CMakeList so that we can make changes when we encounter a distribution that cannot do static linking. |
|
Thank you! For the tests, you are right, we can not test all distributions. Covering the main ones, and leaving the instructions for them, should help others fix the issue with their distribution. That's why I wanted to add the static flag in the documentation, as I couldn't find that detail while building. I thought I needed to recompile gcc tools for that (not fun) While talking about testing, I was thinking that we could add github CI to the repo. This is also related to #144, I couldn't figure out which commit corresponds to the changes that breaks the I can work on the github CI once #144 is resolved. The CI itself should be simple, but I can not work on it now because I don't know if the test is actually failing because of error or the results need to be updated (because the estimates are better now). The first option seems to be the likely case. |
|
I think we should remove it for now because the new CMakeList file looks a
little different but we may need to revisit this later based on the new
CMakeList.txt file and provide instructions for when someone is having
trouble trying to statically link. Also to notify us what linux
distribution they are using so we can fix CMakeList.txt to account for it.
…On Mon, Jan 5, 2026 at 4:01 PM Zero ***@***.***> wrote:
*Atreyagaurav* left a comment (swat-model/swatplus#143)
<#143 (comment)>
Thank you for the review.
I have replaced the use of Exp with exp_w.
As for the CMakeLists.txt, that change was only for explaining what I did
to compile it, the change is not included in the commit. Do you want me to
remove the added portion in the build documentation? I think it might be ok
to leave it here, and remove it when OS specific flags are implemented.
Let me know if I need to change anything else.
—
Reply to this email directly, view it on GitHub
<#143 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2J6UF3CBJG3NPHTL7GQO634FLUNLAVCNFSM6AAAAACPASNEKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMJSGM3TSNJVGI>
.
You are receiving this because your review was requested.Message ID:
***@***.***>
--
Thanks
Frank Geter
|
|
Atreyagaurav
One of the issues is the swatplus main repo releases are ancient and badly
out of date. Olaf talked to Jeff Arnold yesterday about the need to cut a
new release. I think that is going to happen soon. It might be good to
have a meeting on the testing issue. We have talked internally about
automating some tests in GitHub that would have to pass before a release is
built. I'm not sure what github CI entails. Olaf is the github expert and
we would need to engage him on that.
Thanks
Frank
…On Wed, Jan 7, 2026 at 2:58 PM Zero ***@***.***> wrote:
*Atreyagaurav* left a comment (swat-model/swatplus#143)
<#143 (comment)>
Thank you!
For the tests, you are right, we can not test all distributions. Covering
the main ones, and leaving the instructions for them, should help others
fix the issue with their distribution. That's why I wanted to add the
static flag in the documentation, as I couldn't find that detail while
building. I thought I needed to recompile gcc tools for that (not fun)
While talking about testing, I was thinking that we could add github CI to
the repo. This is also related to #144
<#144>, I couldn't figure
out which commit corresponds to the changes that breaks the ctest from
the history. So having CI should help with that. That way we could test at
least windows/mac and ubuntu. Not sure if we can test multiple distro that
way. But just making sure it compiles and runs the test should be a good
plus.
I can work on the github CI once #144
<#144> is resolved. The CI
itself should be simple, but I can not work on it now because I don't know
if the test is actually failing because of error or the results need to be
updated (because the estimates are better now). The first option seems to
be the likely case.
—
Reply to this email directly, view it on GitHub
<#143 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/A2J6UF2UMTOHDFEN2LBCBZT4FV6OXAVCNFSM6AAAAACPASNEKOVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZTOMRQHE3TMOJZGA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
--
Thanks
Frank Geter
|
Fixes: #123
Fixes: #142
Tested with these changes that are not in the commit (due to the unavailability of the static library):
The test runs completely now (no floating point error), but the test fails due to the values being different. I only tested it in
gfortranso please verify this does not break anything forifxcompiler.The log is attached: LastTest.log