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 parsing of gfortran version #47352

Merged
merged 5 commits into from
Nov 15, 2022
Merged

Conversation

apaz-cli
Copy link
Member

No description provided.

@vchuravy vchuravy requested a review from staticfloat October 27, 2022 18:47
@apaz-cli
Copy link
Member Author

I'll return to this, probably want to rewrite the regex more completely to make sure it's sane, but this is enough to solve my issue for now.

@vtjnash
Copy link
Member

vtjnash commented Oct 27, 2022

Could you add your example gfortran --version output to the thread here for demonstration?

@apaz-cli
Copy link
Member Author

This is what x86_64-w64-mingw32-gfortran-posix --version gives me.

GNU Fortran (GCC) 10-posix 20220324
Copyright (C) 2020 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

@apaz-cli apaz-cli changed the title Fix parsing of gfortran version [Draft] Fix parsing of gfortran version Oct 31, 2022
@giordano giordano marked this pull request as draft October 31, 2022 20:04
@ViralBShah
Copy link
Member

Mine is:

PS C:\Users\viral\julia> x86_64-w64-mingw32-gfortran --version
GNU Fortran (GCC) 11.3.0
Copyright (C) 2021 Free Software Foundation, Inc.
This is free software; see the source for copying conditions.  There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is still selecting the last thing that matches, I believe.

@staticfloat
Copy link
Member

IMO it’s a little unreasonable to parse the version number 10-posix. There’s no way we can realistically disambiguate whether 10-posix is the version number, or 20220324 is. I think we should require at least one period in the version number.

@staticfloat
Copy link
Member

If it can’t be auto detected, we should have a good error message, or just default to libgfortran5, since that is by far the most common these days.

@gbaraldi
Copy link
Member

gbaraldi commented Nov 6, 2022

Could we anchor the version search by finding (GCC) and going right from there? Or are there some where it doesn't do that?
We can't :). At least not trivially

gfortran --version
GNU Fortran (Homebrew GCC 12.2.0) 12.2.0

@vtjnash
Copy link
Member

vtjnash commented Nov 8, 2022

Looked into this more, and it appears that --version doesn't really print the version, but some other random stuff. But rather -dumpversion does print the version:

$ gfortran -v 2>&1 | grep 'gcc version'
gcc version 9.4.0 (Ubuntu 9.4.0-1ubuntu1~20.04.1) 
gfortran -dumpversion
9

or

$ gfortran -dumpspecs | grep -C1 "^\*version:\$"

*version:
9.4.0

@vtjnash
Copy link
Member

vtjnash commented Nov 8, 2022

We can't :). At least not trivially

Possibly not even non-trivially haha:

$ gfortran --version
GNU Fortran (MacPorts gcc11 11.3.0_4+stdlib_flag) 11.3.0

@ViralBShah
Copy link
Member

ViralBShah commented Nov 8, 2022

If we don't need gfortran to build, why bother looking for it (at least for users who are doing the default and using BB for all the dependencies)?

@ViralBShah ViralBShah added the system:windows Affects only Windows label Nov 8, 2022
@staticfloat
Copy link
Member

If we don't need gfortran to build, why bother looking for it (at least for users who are doing the default and using BB for all the dependencies)?

Because if they have a gfortran locally, anything they build with that gfortran will be locked to a particular ABI. We need to know what ABI that is, so that Julia's dependencies (e.g. OpenBLAS) use the same ABI.

@vtjnash vtjnash marked this pull request as ready for review November 9, 2022 22:55
@apaz-cli apaz-cli changed the title [Draft] Fix parsing of gfortran version Fix parsing of gfortran version Nov 9, 2022
@vtjnash
Copy link
Member

vtjnash commented Nov 10, 2022

Wow, who would have thought this too could still have so many problems with it

For example:
ValueError: invalid literal for int() with base 10: 'gcc (GCC) 9.1.0'

Or in other cases trying to interpret the Apple version of clang as a gfortan compiler

@vtjnash
Copy link
Member

vtjnash commented Nov 10, 2022

Apparently supporting newer versions need gcc -dumpfullversion -dumpversion according to stack overflow

@vtjnash
Copy link
Member

vtjnash commented Nov 10, 2022

The various dump flags are defined here (for their current form, after getting renamed in gcc-8) https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Developer-Options.html#Developer-Options

If all else fails, we can get the all cpp macros with -dM -E and search for __GNUC__ in them. That API at least has been around and reliable for a long time.

@apaz-cli
Copy link
Member Author

Is there a case where the major version number would ever not be the first number in gfortran -dumpversion?

@vtjnash
Copy link
Member

vtjnash commented Nov 10, 2022

Yes, the docs clarify that this was changed from being the fullversion to being the filepath name

Make.inc Outdated Show resolved Hide resolved
Co-authored-by: Jameson Nash <[email protected]>
@vtjnash vtjnash requested a review from staticfloat November 15, 2022 14:54
Copy link
Member

@staticfloat staticfloat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

@vtjnash vtjnash merged commit 626f3b2 into JuliaLang:master Nov 15, 2022
@KristofferC KristofferC added the backport 1.6 Change should be backported to release-1.6 label Dec 21, 2022
KristofferC pushed a commit that referenced this pull request Dec 21, 2022
Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 626f3b2)
staticfloat pushed a commit that referenced this pull request Dec 23, 2022
Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 626f3b2)
KristofferC pushed a commit that referenced this pull request Oct 10, 2023
Co-authored-by: Jameson Nash <[email protected]>
(cherry picked from commit 626f3b2)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport 1.6 Change should be backported to release-1.6 system:windows Affects only Windows
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants