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

Bugs encountered when trying to bootstrap with intel ifort 2021.7 #772

Closed
rweed opened this issue Oct 8, 2022 · 4 comments · Fixed by #804
Closed

Bugs encountered when trying to bootstrap with intel ifort 2021.7 #772

rweed opened this issue Oct 8, 2022 · 4 comments · Fixed by #804
Labels
bug Something isn't working

Comments

@rweed
Copy link

rweed commented Oct 8, 2022

Description

It appears that with ifort 2021.7 the word "unix" is predefined as a macro. Therefore in functions like os_is_linux and other places where the word "unix" is used. It is reset by the pre-processor to "1". ie.

logical function os_is_unix(os) result(unix)
.
.
unix = build_os /= OS_WINDOWS
end function os_is_unix becomes

Becomes

logical function os_is_unix(os) result(1)
.
.
1 = build_os /= OS_WINDOWS
end function os_is_unix

I got everything to compile by changing all occurences of the logical variable unix to unix_ but
I still get a couple of warnings.

ie

fpm-0.6.0.F90(5150): remark #8291: Recommended relationship between field width 'W' and the number of fractional digits 'D' in this edit descriptor is 'W>=D+7'.
read(nan_string,'(g3.3)')valu
----------------------------^
fpm-0.6.0.F90(13432): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value. [SELF]
subroutine new_keyval(self)
----------------------^
ld: /tmp/ifortUNxRu0.o: in function fpm_filesystem_mp_get_temp_filename_': /mnt/Work/FPM/fpm-0.6.0.F90:10050: warning: the use of tempnam' is dangerous, better use `mkstemp'

and yes I know I could use gfortran but I prefer to use intel.

Expected Behaviour

It should have compiled from the start

Version of fpm

0.6.0 fpm-0.6.0.F90

Platform and Architecture

Linux Mint 20.3 (Ubuntu 20.04 base) X86-64

Additional Information

No response

@rweed rweed added the bug Something isn't working label Oct 8, 2022
@awvwgk
Copy link
Member

awvwgk commented Oct 8, 2022

Adding an undefine command to the flags should work around this

❯ FC=ifort CC=icc FFLAGS=-Uunix ./install.sh --prefix=$PWD/_install

We can also include #undef unix at

echo "#define FPM_BOOTSTRAP" > fpm-${{ env.VERSION }}.F90

to make this automatic.

@awvwgk
Copy link
Member

awvwgk commented Oct 8, 2022

fpm-0.6.0.F90(5150): remark #8291: Recommended relationship between field width 'W' and the number of fractional digits 'D' in this edit descriptor is 'W>=D+7'.
read(nan_string,'(g3.3)')valu
----------------------------^

That's something to report to M_CLI2, see

https://github.com/urbanjost/M_CLI2/blob/9d4cc99dcf68b9408531824c7ec9c1415a7f5b7e/src/M_CLI2.F90#L3790

fpm-0.6.0.F90(13432): warning #6843: A dummy argument with an explicit INTENT(OUT) declaration is not given an explicit value. [SELF]
subroutine new_keyval(self)
----------------------^

That looks like an issue with TOML Fortran, see

https://github.com/toml-f/toml-f/blob/e5f04f9595c357fa15e07cfc7ead359c4213bf1b/src/tomlf/type/keyval.f90#L101-L109

ld: /tmp/ifortUNxRu0.o: in function `fpm_filesystem_mp_get_temp_filename_': /mnt/Work/FPM/fpm-0.6.0.F90:10050: warning: the use of `tempnam' is dangerous, better use `mkstemp'

Already reported in #697

@urbanjost
Copy link
Contributor

#define FPM_BOOTSTRAP
#undef linux
#undef unix

Thought the standalone version had #undef in it. I generate my own standalone version for various reasons, and the build does an undef of "linux" and "unix".

The warning (not error) about the field widths is generated by many valid Fortran statements and inconsistently does not generate the warning when f3.3 is used instead of g3.3 so I could change it to that to remove the extraneous warning, but fpm tags a specific version of M_CLI2 so it would take a PR fo fpm as well. It does not prevent the ifort/ifx builds from working, and the format g3.3 is exactly the statement I want there. DId it actually prevent compilation (maybe it does in some version I do not use (?))?

@awvwgk awvwgk linked a pull request Nov 26, 2022 that will close this issue
@urbanjost
Copy link
Contributor

A footnote that the "linux" and "unix" predefined macros have been submitted to the Intel forum as an issue. The macros have been undefined in the standalone file, but the user is required to undefine them when doing an ifort build, as in

     fpm build --compiler ifort --profile release --flag '-Uunix -Ulinux' --verbose 

As at least IMHO that is a bug in ifort, and should not require adding an #undef in every preprocessed file, particularly because
you can specify that all files, not just files with a .F90 suffix be preprocessed (so the #undef would not just have to be in files ending in .F90 but all files, and if the user was NOT preprocessing all files those directives would cause errors). Adding -Uunix -Ulinux flags to the default ifort flags would also have drawbacks. So a build with ifort of the fpm package now works with the
above syntax, and the standalone file (which at least currently always requires preprocessing and is named "fpm.F90") is done
now.

The standalone version builds with a simple "ifort fpm.F90" now.

The issue has not yet got much comment, but can be seen at the following link, and the initial post is listed here for reference.
Any additional comments on that specific issue should be posted on the Intel forum to further the ongoing discussion ...

ur
‎11-27-2022 09:24 AM

On multiple occasions the predefined macros "linux" and "unix" have caused porting problems.

Such common terms without an underscore prefix and/or suffix have caused inadvertent substitution on multiple occasions. I was hoping ifx in particular would not carry this forward, but apparently it does; including "unix" which does not appear in either the ifort or ifx man-pages as

a predefined macro.

 

At least in ifx going forward I think these should be removed. If not removed "unix" should be documented at a minimum.

 

Usually, it causes compilation failures, and new users in particular are confused. Turning off macro substitution, adding #undef, or -U can all work around it, but I have seen it cause issues multiple times, particularly when build utilities/IDEs run all files through the preprocessor regardless of file suffix.

 

Perhaps a warning to switch to __linux and __unix could be produced if it appears in a preprocessor directive would be useful, but such common terms should never be predefined, particularly in the case of ifx which gives an opportunity to quit carrying this forward.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants