-
Notifications
You must be signed in to change notification settings - Fork 107
New issue
Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? # to your account
fix 32-bit platform issues with M_CLI2 #796
Conversation
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.
LGTM. @urbanjost does this come with any additional consequences? I had a look at the git-diff and it is rather big but in the CHANGELOG I only saw the bug you mention in this PR.
If you do the git-diff of the entire project it shows many files that are auto-generated by ford and doxygen; which users have asked me to leave in the distribution as a source of documentation for their down-wind projects so they do not have to install and run infrastructure they do not need otherwise. The only file that matters is M_CLI2.f90. It has a lot of trivial changes because of a style change tool that did things like change ".eq." to "==", and because the source files are generated from prep(1) input files, and some changes in how built-in help text is treated many comment lines changed. So in the one file that matters there are no significant changes except one other which was for an experimental attempt to allow response files to be compatible with the Intel compiler so that prep(1) could be used as a substitute for fpp(1) as the document implies is supported, but after discussion it really is not. Apparently we were the first ones to actually try having ifort(1) call our own custom pre-processor. But those changes no not affect current usage; they are only triggered if you use a response file and have nothing in it but command options and do not follow the M_CLI2 response file format. That is because one of the issues we found trying to integrate prep(1) into ifort(1) was that (undocumented) ifort(1) does not pass it's options on the command line, but via a simple response file. So there are no other significant changes in M_CLI2.f90 except the response file change, which does not affect current usage; and what you see are artifacts; so the only user-facing change is the one in the CHANGELOG.md file. I diff a diff between the M_CLI2.f90 from the two versions and went through it manually. It is mostly just minor comment and style changes with the exceptions noted above; although it shows a lot of diff output, there are no (intentionally :>) significant changes otherwise. |
Great, thanks for the detailed response @urbanjost I really appreciate it. I'll go ahead and approve. I don't know if @zoziha and @LKedward want to have a look as well. |
Great. I was encouraged to see, as I understand it, that fpm is being added as a supported 3rd-party package as part of OpenBSD and this is a snag in deploying it on the i386 version. I have been using the fix in a private copy of fpm for several days on OpenBSD/i386 and have not had any subsequent issues with argument parsing; but using it to build my public github/fpm packages I have found a few other issues that appear to be egfortran(1) issues, not fpm(1) issues. A good number of packages build and pass their confidence and/or QA, but several hit what appear to be compiler bugs. I want to report the compiler bugs as well to make that a better Fortran environment but have only had time for one so far. Have not hit any problems on the 64-bit version of OpenBSD so far; but this bug does not affect any 64-bit platform I know of, although it is not impossible that is would,depending on rounding mode and how the LOG() intrinsic is implemented. |
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.
All CI checks passed, LGTM, thanks for this patch.
In the meantime had an issue posted for M_CLI2 that, unlike what the documentation said on MSWindows fpm.rsp is not searched for as the compound response file (when using powershell and MSYS2; worked in a cmd window and default environment) but fpm.exe.rsp. So another slight change to trim .* suffix from the executable name and to allow an environment variable to trigger the M_CLI2 debug mode to simplify future testing when issues arise, instead of requiring a recompile. So now, whether the executable is called fpm.exe or fpm the same response file (fpm.rsp) is searched for by default. Response files were somewhat seen as a stopgap until fpm.toml profiles were supported and is barely mentioned in the fpm documentation. It seems to have since popular. |
Tests of fpm on i-386 versions of OpenBSD exposed an error in the M_CLI2 procedure locate_c where, when calculating the maximum number of times to split the search in a binary search for keywords the function INT instead of NINT was used; which on 32-bit platforms caused the value to be low by one due to truncation. Newer versions of M_CLI2 has this fixed. Changing the fpm.toml file to use a newer version
should fix #795