Skip to content

Publish version to within fpm #865

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

Merged
merged 32 commits into from
Apr 14, 2023
Merged

Conversation

perazz
Copy link
Member

@perazz perazz commented Apr 3, 2023

Addressing #855: for metapackages and registry handling, fpm must know what version is running.

I think there are two main ways of doing this:

  1. pass it via a pre-processor flag
  2. read a generic external file
  3. pass it via a fortran include file.

In both cases, a CI script needs to keep this version up to date.

I like option 3) better because it's not compiler dependent, and once the repo is deployed, there's no way to change it, it's inside the source code. So here's what I think may work:

  • Extract version from git in the wake of what LFortran does: 09d09e1

  • Have all fpm include files inside include/, so they're excluded from the build: c9a35da

  • I think none of these parameters should be public, just operate with version_t within fpm 44b0a2a

If there's consensus on this way forward @certik @awvwgk @urbanjost @fortran-lang/admins, please let me know and I can complete the CI and the tests! I would then address fpm version requirements as a separate PR.

cc: @arteevraina @henilp105 @minhqdao

@perazz
Copy link
Member Author

perazz commented Apr 6, 2023

include files are not supported by the CI when it builds a single-file source of fpm. The solution is nontrivial so I've changed the CI script to create a full Fortran module file where all CI-based parameters will be stored. Looks much better IMHO.

src/fpm/fpm_release_parameters.f90

awvwgk
awvwgk previously requested changes Apr 6, 2023
Copy link
Member

@awvwgk awvwgk left a comment

Choose a reason for hiding this comment

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

I strongly believe that this is not the way to go. To export the version from the from must not rely on an external script, rather fpm should support generating a module like this intrinsically.

@perazz
Copy link
Member Author

perazz commented Apr 6, 2023

I understand @awvwgk, what is your piece of advice? You mean fpm should generate that module file and add it somewhere as a dependency to the child fpm build? That's of course feasible, maybe easier than with a bash script, but it would have to read the version and commit ID starting from a CI variable or pre-processing script anyways?

@awvwgk
Copy link
Member

awvwgk commented Apr 6, 2023

That's probably a longer discussion. To export the version from only needs to know the manifest and we already have a preprocess table to forward a version number macro. Generally I don't see any good reason to pick up the version number from a VCS tag, stuff like setuptools-scm introduce quite some edge cases in the Python ecosystem already.

fpm.toml Outdated
Comment on lines 14 to 17
[preprocess]
[preprocess.cpp]
macros=["FPM_VERSION={version}"]

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 following update passes version from the manifest into fpm as a preprocessed macro @awvwgk. I like this way, but it cannot be built (fpm 0.3.0 used for bootstrapping). Should we update fpm to 0.7.0 in the bootstrapping action?

@perazz
Copy link
Member Author

perazz commented Apr 7, 2023

@awvwgk It seems like fpm cannot be bootstrap with fpm-0.7.0 release: the CI crashes due to bug #803 in pre-processing flags which also exists in latest. One option would be to setup the fpm-setup action with a more recent build, but that is very strongly discouraged IMHO

@awvwgk
Copy link
Member

awvwgk commented Apr 7, 2023

Then let's make a new release 0.7.1 which picks the bugfix or a new feature release 0.8.0

Comment on lines +55 to +61
echo ${{ env.VERSION }}

- name: Get manifest version
run: |
MANIFEST_VERSION=$(grep version fpm.toml | head -1 | tr -d ' ' | tr -d 'version=')
echo "MANIFEST_VERSION=${MANIFEST_VERSION}" >> $GITHUB_ENV
echo ${{ env.MANIFEST_VERSION }}
Copy link
Member Author

@perazz perazz Apr 7, 2023

Choose a reason for hiding this comment

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

Using pre-processing flags, there are a couple more steps with the single-file-version:

  1. the single-source-file version is not built with fpm, so we need to first query the manifest version into a variable,

Comment on lines 78 to 79
# We need to pass the exact version string that a fpm build command would send
echo "#define FPM_RELEASE_VERSION ${{ env.MANIFEST_VERSION }}" > fpm-${{ env.VERSION }}.F90
Copy link
Member Author

@perazz perazz Apr 7, 2023

Choose a reason for hiding this comment

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

  1. manually set a preprocessor flag.

Comment on lines 19 to 38
! Accept solution from https://stackoverflow.com/questions/31649691/stringify-macro-with-gnu-gfortran
! which provides the "easiest" way to pass a macro to a string in Fortran complying with both
! gfortran's "traditional" cpp and the standard cpp syntaxes

#ifndef FPM_RELEASE_VERSION
# define FPM_RELEASE_VERSION UNDEFINED
#endif

#ifdef __GFORTRAN__ /* traditional-cpp stringification */
# define STRINGIFY_START(X) "&
# define STRINGIFY_END(X) &X"
#else /* default stringification */
# define STRINGIFY_(X) #X
# define STRINGIFY_START(X) &
# define STRINGIFY_END(X) STRINGIFY_(X)
#endif

character (len=:), allocatable :: ver_string
ver_string = STRINGIFY_START(FPM_RELEASE_VERSION)
STRINGIFY_END(FPM_RELEASE_VERSION)
Copy link
Member Author

Choose a reason for hiding this comment

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

Finally, gfortran needs some pretty verbose preprocessing, even just to pass along a string, because it uses the "traditional" version of the cpp preprocessor

@perazz
Copy link
Member Author

perazz commented Apr 7, 2023

I believe this is completed. @henilp105 @arteevraina @minhqdao @awvwgk @certik if any of you needs this merged before the weekend, please review it and do it yourself, I'll be back on Tuesday. Happy Easter!

@awvwgk awvwgk dismissed their stale review April 7, 2023 14:51

Requested changes are made

Copy link
Member

@henilp105 henilp105 left a comment

Choose a reason for hiding this comment

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

Thanks @perazz , Looks good to me.

@perazz
Copy link
Member Author

perazz commented Apr 13, 2023

I would like to merge this one so we can publish the fpm version to the model dump (#879). I will wait another day or so and if there are no further requests, will go ahead and merge.

@perazz
Copy link
Member Author

perazz commented Apr 14, 2023

Thank you @henilp105, I will merge this one.

@perazz perazz merged commit 70e32c9 into fortran-lang:main Apr 14, 2023
@perazz perazz deleted the publish_version_to_fpm branch April 14, 2023 11:10
This was referenced Apr 19, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

3 participants