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

Add module for handling version information of stdlib #579

Merged
merged 8 commits into from
Nov 27, 2021

Conversation

awvwgk
Copy link
Member

@awvwgk awvwgk commented Nov 25, 2021

Closes #576

Co-authored-by: Ivan Pribec <[email protected]>
Comment on lines +12 to +14
PROJECT_VERSION_MAJOR=0
PROJECT_VERSION_MINOR=0
PROJECT_VERSION_PATCH=0
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
PROJECT_VERSION_MAJOR=0
PROJECT_VERSION_MINOR=0
PROJECT_VERSION_PATCH=0
PROJECT_VERSION_MAJOR=0
PROJECT_VERSION_MINOR=1
PROJECT_VERSION_PATCH=0

Copy link
Member

Choose a reason for hiding this comment

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

Or is this overridden during the fypp processing?

Copy link
Member Author

Choose a reason for hiding this comment

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

This is supposed to be a stub, similar like the MAXRANK=3 argument above, which would would not allow to actually compile stdlib.

Copy link
Member

Choose a reason for hiding this comment

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

Don't these macros get applied when FORD runs fypp to preprocess the sources for the documentation? At least that's what I understand from here: https://github.com/Fortran-FOSS-Programmers/ford/wiki/Project-File-Options

Copy link
Member Author

Choose a reason for hiding this comment

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

It does show up in the final docs for the compile time constant:

image

Otherwise the value set won't affect the output of the documentation. I would prefer to leave this as a stub, so we don't have to worry about it becoming outdated when we release a new version.

Copy link
Member

Choose a reason for hiding this comment

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

Comments are not allowed in the FORD meta-data section. Is there somewhere we could document these values are stubs, and don't reflect the actual library?

Co-authored-by: Ivan Pribec <[email protected]>
Copy link
Member

@ivan-pi ivan-pi left a comment

Choose a reason for hiding this comment

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

The CMake syntax to read a list is just terrible.

Pending the small issues, LGTM. Thanks.

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@awvwgk many thanks for this PR. I tested it on Windows using CMake+Ninja.
Regarding the issue with $(shell cut ..), I'm trying to find a solution but no luck yet.

@ghost
Copy link

ghost commented Nov 26, 2021

OK I found a combination that works for me on Windows

set FC=gfortran
set FYPPFLAGS=-DMAXRANK=4
gnumake -f Makefile.manual

Thanks @awvwgk

Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

Thanks @awvwgk

@ghost
Copy link

ghost commented Nov 26, 2021

I just noticed ubuntu-latest CI is failing at make -f Makefile.manual FYPPFLAGS="-DMAXRANK=4" -j. It is the same issue. If we remove FYPPFLAGS="-DMAXRANK=4" from the command line and set it as environment variable, it will work, I think. That's how I did on my Windows.

@awvwgk awvwgk marked this pull request as ready for review November 26, 2021 11:20
`major`: shall be an intrinsic integer type. It is an optional, `intent(out)` argument.
`minor`: shall be an intrinsic integer type. It is an optional, `intent(out)` argument.
`patch`: shall be an intrinsic integer type. It is an optional, `intent(out)` argument.
`string`: shall be a deferred length character type. It is an optional, `intent(out)` argument.
Copy link
Member

Choose a reason for hiding this comment

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

Just wondering if a fixed-length string can also be used to collect the output?

Copy link
Member Author

Choose a reason for hiding this comment

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

The dummy argument is currently character(len=:), allocatable, intent(out), optional, since we are using a subroutine rather than a function. The rationale is that we want a stable API and ABI for this procedure, we could also go for multiple functions for getting the individual values, to make use of it in a more functional way.

Co-authored-by: Ivan Pribec <[email protected]>
Copy link

@ghost ghost left a comment

Choose a reason for hiding this comment

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

@awvwgk looks good.

@ivan-pi
Copy link
Member

ivan-pi commented Nov 27, 2021

@jvdp1, @milancurcic, can one of you give this a third approval before merging?

@awvwgk awvwgk requested a review from a team November 27, 2021 14:05
Copy link
Member

@jvdp1 jvdp1 left a comment

Choose a reason for hiding this comment

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

LGTM. Just a couple of minor comments.
Thank you!

Comment on lines +13 to +14
In case the standard library is dynamically linked, the version number retrieved from the getter might mismatch the compile time constants provided from the version built against.
Therefore, it is recommended to retrieve the version information always at runtime.
Copy link
Member

Choose a reason for hiding this comment

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

Why not only allow the correct approach to do it?

Copy link
Member Author

@awvwgk awvwgk Nov 27, 2021

Choose a reason for hiding this comment

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

For the compact version I can see an application to verify the compatibility of API and/or ABI at runtime by a construct like:

use stdlib_version, only : stdlib_version_compact, get_stdlib_version
implicit none
integer :: major
call get_stdlib_version(major=major)
if (major > stdlib_version_compact / 10000) then
  error stop "Incompatible version of stdlib used"
end if
end

Similarly, stdlib_version_string and get_stdlib_version(string=string) can be used to provide a printout to inform about the stdlib versions used at build time and run time.

Not sure whether this is a realistic application of this information, usually this kind of verification happens at build time and constraints in package managers can ensure compatible version, but not everybody is using those.

Copy link
Member

Choose a reason for hiding this comment

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

Ok. Thank you for the explanation. You can merge this PR from my point-of-view.

Co-authored-by: Jeremie Vandenplas <[email protected]>
@awvwgk awvwgk merged commit 3af3259 into fortran-lang:master Nov 27, 2021
@awvwgk awvwgk deleted the version-info branch November 27, 2021 17:45
Copy link
Member

@milancurcic milancurcic left a comment

Choose a reason for hiding this comment

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

Very nice addition, thank you.

Would it be useful for this module to also provide compile-time constants of compiler version and options used to build it?

module m
  use iso_fortran_env, only: compiler_version, compiler_options
  character(*), parameter :: stdlib_compiler_version = compiler_version()
  character(*), parameter :: stdlib_compiler_options = compiler_options()
end module m

program p
  use m
  print *, stdlib_compiler_version
  print *, stdlib_compiler_options
end

It works nicely with GFortran, but ifort 2021.3.0 doesn't seem to allow these intrinsic in constant expressions:

$ ifort test.f90 
test.f90(3): error #6263: This intrinsic function is invalid in constant expressions.   [COMPILER_VERSION]
  character(*), parameter :: stdlib_compiler_version = compiler_version()
-------------------------------------------------------^
test.f90(4): error #6263: This intrinsic function is invalid in constant expressions.   [COMPILER_OPTIONS]
  character(*), parameter :: stdlib_compiler_options = compiler_options()
-------------------------------------------------------^
test.f90(8): error #7013: This module file was not generated by any release of this compiler.   [M]
  use m
------^
compilation aborted for test.f90 (code 1)

which I think is incorrect. Anyhow, just an idea, something we could tackle in another PR.

@awvwgk
Copy link
Member Author

awvwgk commented Nov 27, 2021

Good idea, I opened a new issue for this one.

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.

stdlib version information function
4 participants