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

Include H5Version.hpp in the sources. #819

Merged
merged 1 commit into from
Oct 31, 2023
Merged

Include H5Version.hpp in the sources. #819

merged 1 commit into from
Oct 31, 2023

Conversation

1uc
Copy link
Collaborator

@1uc 1uc commented Aug 4, 2023

Generated files require users to "build" and "install" HighFive, for the sole purpose of having files generated via configure_file available. The only file that generated in HighFive is the version file.

This PR proposes to automatically generate the file but also commit it to the repo. The advantage of doing so, is that users can clone the repo, set the appropriate value for -I and they're done (if they don't need any addons).

In order to check that the version number in CMakeLists.txt and H5Version.hpp are consistent we configure HighFive during CI and check that the file hasn't changed.

@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

❗ No coverage uploaded for pull request base (master@c04fdf1). Click here to learn what that means.
Patch has no changes to coverable lines.

Additional details and impacted files
@@            Coverage Diff            @@
##             master     #819   +/-   ##
=========================================
  Coverage          ?   84.59%           
=========================================
  Files             ?       68           
  Lines             ?     4784           
  Branches          ?        0           
=========================================
  Hits              ?     4047           
  Misses            ?      737           
  Partials          ?        0           

☔ View full report in Codecov by Sentry.

📢 Have feedback on the report? Share it here.

@1uc 1uc force-pushed the 1uc/version-file branch 2 times, most recently from 92e1aa0 to c417e6a Compare August 4, 2023 09:22
@1uc
Copy link
Collaborator Author

1uc commented Aug 4, 2023

Here the new CI ran with an outdated H5Version.hpp:
https://github.com/BlueBrain/HighFive/actions/runs/5760522600/job/15616631667

@1uc 1uc marked this pull request as ready for review September 8, 2023 05:59
run: |
# Check that the file hasn't changed, i.e. was updated
# after changing the version number.
! git status | grep include/highfive/H5Version.hpp
Copy link
Member

Choose a reason for hiding this comment

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

Would be nice if we could ensure that the version is also greater or equal to the latest tag.

Copy link
Collaborator Author

@1uc 1uc Sep 15, 2023

Choose a reason for hiding this comment

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

I think understand the motivation. It would be nice to have something that checks we didn't forget to bump the version number.

Our current workflow is to:

  1. Create a commit which updates the change log and bumps the version.
  2. Create a PR.
  3. Merge the PR.
  4. Tag the appropriate commit on master.

Therefore a naive solution has two issues:

  • There's a brief period where the version number is strictly larger than the latest tag. Which requires as you say, that we check for greater or equal.
  • Due to the greater or equal it'll initially succeed even if we forget to bump the version number. Only after we tag a commit, will it complain. That too late for my taste.

Instead it would be nicer to have a CD pipeline to create a release, it will change the version in CMakeLists.txt and tag consistently.

Copy link
Member

Choose a reason for hiding this comment

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

Instead it would be nicer to have a CD pipeline to create a release, it will change the version in CMakeLists.txt and tag consistently.

Yes, agreed. We can put this in a virtual backlog, let's get this in first.

@1uc 1uc merged commit 386d1b6 into master Oct 31, 2023
36 checks passed
@1uc 1uc deleted the 1uc/version-file branch October 31, 2023 13:50
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.

2 participants