Skip to content
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

fix 32-bit platform issues with M_CLI2 #796

Merged
merged 2 commits into from
Nov 24, 2022
Merged

fix 32-bit platform issues with M_CLI2 #796

merged 2 commits into from
Nov 24, 2022

Conversation

urbanjost
Copy link
Contributor

@urbanjost urbanjost commented Nov 16, 2022

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

Copy link
Member

@gnikit gnikit left a 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.

@urbanjost
Copy link
Contributor Author

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.

@gnikit
Copy link
Member

gnikit commented Nov 17, 2022

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.

@urbanjost
Copy link
Contributor Author

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.

urbanjost/M_CLI2#12

Copy link
Contributor

@zoziha zoziha left a 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.

bob-beck pushed a commit to openbsd/ports that referenced this pull request Nov 18, 2022
@gnikit gnikit merged commit 58c7f76 into fortran-lang:main Nov 24, 2022
@urbanjost
Copy link
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

fpm fails on i386 openbsd platform due to error in the locate_c procedure in M_CLI2
3 participants