fix: version for tags with -arabica, -mocha, -rc suffixes
#3977
Conversation
rootulp
requested review from
staheri14,
ninabarbakadze,
cmwaters,
evan-forbes and
rach-id
and removed request for
a team
rootulp
changed the title
-arabica, -mocha, -rc suffixes
📝 WalkthroughWalkthroughThe changes in the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Outside diff range and nitpick comments (2)
Makefile (2)
1-16: Improved versioning logic, consider minor optimizationThe new
GIT_TAGdefinition effectively addresses the version reporting issue by prioritizing specific tag formats. The fallback mechanism ensures robustness.Consider using
head -n1instead of multiplegrepcommands to potentially improve performance:-GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' \ - || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-mocha$$' \ - || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-arabica$$' \ - || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]*$$' \ - || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(beta)$$' \ - || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(alpha)$$' \ - || git describe --tags) +GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+(-mocha|-arabica|-rc[0-9]*|-beta|-alpha)?$$' | head -n1 \ + || git describe --tags)This change combines all the patterns into a single
grepcommand and useshead -n1to select the first matching tag, potentially improving performance while maintaining the same functionality.
276-280: Useful debug-version target addedThe new
debug-versiontarget provides a convenient way to verify theGIT_TAGandVERSIONvalues, which is helpful for debugging version-related issues.Consider adding a separator between the two echo commands for better readability of the output:
debug-version: @echo "GIT_TAG: $(GIT_TAG)" + @echo "------------------------" @echo "VERSION: $(VERSION)" .PHONY: debug-versionThis change will make it easier to distinguish between the two values in the output.
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Makefile (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Makefile (2)
17-17: Correct VERSION derivation from GIT_TAGThe
VERSIONvariable is correctly derived fromGIT_TAGby removing the leading 'v'. This ensures that the version is reported in the expected format.
Line range hint
1-280: Overall improvements to version handling and debuggingThe changes in this Makefile effectively address the version reporting issues mentioned in the PR objectives. The new
GIT_TAGvariable provides a robust method for determining the correct version tag, while theVERSIONvariable ensures the proper format. The addition of thedebug-versiontarget enhances maintainability by providing an easy way to verify version information.These modifications significantly improve the reliability and accuracy of version reporting in the
celestia-appdapplication, aligning well with the goals outlined in issue #3933.
📝 WalkthroughWalkthroughThe changes in this pull request primarily involve modifications to the Changes
Assessment against linked issues
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
Documentation and Community
|
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: .coderabbit.yaml
Review profile: CHILL
📒 Files selected for processing (1)
- Makefile (2 hunks)
🧰 Additional context used
🔇 Additional comments (2)
Makefile (2)
276-280: Approved: Addition ofdebug-versiontargetThe
debug-versiontarget is a useful addition that allows developers to easily check theGIT_TAGandVERSIONvalues. The implementation is correct and follows standard Makefile practices.
10-16: Verify tag matching patterns for completenessEnsure that the consolidated regular expression accurately matches all intended tag formats and maintains the priority order. There could be edge cases where certain tags might not be matched correctly.
You can test the regular expression with various tag examples to confirm it captures all desired formats:
This script helps verify that all intended tag formats are matched and unintended ones are excluded.
✅ Verification successful
Tag patterns are correctly matched and prioritized
All intended tag formats are accurately matched by the regular expression, and unintended tags are appropriately excluded.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Description: Test the consolidated regular expression against sample tags. tags=( "v2.2.0" "v2.2.0-mocha" "v2.2.0-arabica" "v2.2.0-rc0" "v2.2.0-beta" "v2.2.0-alpha" "v2.2.0-unknown" ) for tag in "${tags[@]}"; do echo "$tag" | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+(-mocha|-arabica|-rc[0-9]+|-beta|-alpha)?$$' && echo "Matched: $tag" doneLength of output: 1360
Script:
#!/bin/bash # Description: Test the consolidated regular expression against sample tags. tags=( "v2.2.0" "v2.2.0-mocha" "v2.2.0-arabica" "v2.2.0-rc0" "v2.2.0-beta" "v2.2.0-alpha" "v2.2.0-unknown" ) for tag in "${tags[@]}"; do if echo "$tag" | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+(-mocha|-arabica|-rc[0-9]+|-beta|-alpha)?$' > /dev/null; then echo "Matched: $tag" else echo "Did not match: $tag" fi doneLength of output: 1351
| # GIT_TAG is an environment variable that is set to the latest git tag on the | ||
| # current commit with the following example priority: v2.2.0, v2.2.0-mocha, | ||
| # v2.2.0-arabica, v2.2.0-rc0, v2.2.0-beta, v2.2.0-alpha. If no tag points to the | ||
| # current commit, git describe is used. The priority in this command is | ||
| # necessary because `git tag --sort=-creatordate` only works for annotated tags | ||
| # with metadata. Git tags created via GitHub releases are not annotated and do | ||
| # not have metadata like creatordate. Therefore, this command is a hacky attempt | ||
| # to get the most recent tag on the current commit according to Celestia's | ||
| # testnet versioning scheme + SemVer. | ||
| GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-mocha$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-arabica$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]*$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(beta)$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(alpha)$$' \ | ||
| || git describe --tags) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Suggestion: Consolidate git tag commands for efficiency
Currently, the GIT_TAG variable is assigned by invoking git tag --points-at HEAD multiple times, which can be inefficient. Consider capturing the output once and applying all necessary filters in a single command.
Here's a proposed modification:
-GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' \
- || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-mocha$$' \
- || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-arabica$$' \
- || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]*$$' \
- || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(beta)$$' \
- || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(alpha)$$' \
+GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname \
+ | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+(-mocha|-arabica|-rc[0-9]+|-beta|-alpha)?$$' \
|| git describe --tags)This approach reduces the number of times git tag is called and consolidates the pattern matching into a single grep command.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| # GIT_TAG is an environment variable that is set to the latest git tag on the | |
| # current commit with the following example priority: v2.2.0, v2.2.0-mocha, | |
| # v2.2.0-arabica, v2.2.0-rc0, v2.2.0-beta, v2.2.0-alpha. If no tag points to the | |
| # current commit, git describe is used. The priority in this command is | |
| # necessary because `git tag --sort=-creatordate` only works for annotated tags | |
| # with metadata. Git tags created via GitHub releases are not annotated and do | |
| # not have metadata like creatordate. Therefore, this command is a hacky attempt | |
| # to get the most recent tag on the current commit according to Celestia's | |
| # testnet versioning scheme + SemVer. | |
| GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' \ | |
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-mocha$$' \ | |
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-arabica$$' \ | |
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]*$$' \ | |
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(beta)$$' \ | |
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(alpha)$$' \ | |
| || git describe --tags) | |
| # GIT_TAG is an environment variable that is set to the latest git tag on the | |
| # current commit with the following example priority: v2.2.0, v2.2.0-mocha, | |
| # v2.2.0-arabica, v2.2.0-rc0, v2.2.0-beta, v2.2.0-alpha. If no tag points to the | |
| # current commit, git describe is used. The priority in this command is | |
| # necessary because `git tag --sort=-creatordate` only works for annotated tags | |
| # with metadata. Git tags created via GitHub releases are not annotated and do | |
| # not have metadata like creatordate. Therefore, this command is a hacky attempt | |
| # to get the most recent tag on the current commit according to Celestia's | |
| # testnet versioning scheme + SemVer. | |
| GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname \ | |
| | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+(-mocha|-arabica|-rc[0-9]+|-beta|-alpha)?$$' \ | |
| || git describe --tags) |
There was a problem hiding this comment.
Maybe you want to do this ^
There was a problem hiding this comment.
The proposed command won't use the priority we want (v2.2.0, v2.2.0-mocha, v2.2.0-alpha, etc)
| # GIT_TAG is an environment variable that is set to the latest git tag on the | ||
| # current commit with the following example priority: v2.2.0, v2.2.0-mocha, | ||
| # v2.2.0-arabica, v2.2.0-rc0, v2.2.0-beta, v2.2.0-alpha. If no tag points to the | ||
| # current commit, git describe is used. The priority in this command is | ||
| # necessary because `git tag --sort=-creatordate` only works for annotated tags | ||
| # with metadata. Git tags created via GitHub releases are not annotated and do | ||
| # not have metadata like creatordate. Therefore, this command is a hacky attempt | ||
| # to get the most recent tag on the current commit according to Celestia's | ||
| # testnet versioning scheme + SemVer. | ||
| GIT_TAG := $(shell git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-mocha$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-arabica$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-rc[0-9]*$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(beta)$$' \ | ||
| || git tag --points-at HEAD --sort=-v:refname | grep -E '^v[0-9]+\.[0-9]+\.[0-9]+-(alpha)$$' \ | ||
| || git describe --tags) |
There was a problem hiding this comment.
Maybe you want to do this ^
Closes #3933
Testing