-
Notifications
You must be signed in to change notification settings - Fork 6.2k
Correct handling of tagged prereleases #16246
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
Conversation
b3a2170 to
4c773aa
Compare
|
Note that there are several bits of prerelease logic that I did not bother to update here:
|
| else | ||
| date -u +"nightly.%Y.%-m.%-d" > prerelease.txt; | ||
| fi | ||
| echo -n "$CIRCLE_SHA1" > commit_hash.txt |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not really sure if there was a reason we were creating commit_hash.txt here, but it seems completely unnecessary now. It appeared a long time ago in #3470, with no explanation, even though the logic for getting it from git already existed in buildinfo.cmake at that time. Then it spread to other places when this logic was copied.
b4032bd to
f6d6167
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Note that tagged prereleases will skip most of the CI in solc-bin, because compare_bytecode_reports.sh filters out anything that's not a release. I think it's fine, because they are supposed to have passed CI checks in the solidity repo and messing this up is not as big of a deal as with a full release. Still, one thing that would be useful to keep is the sanity check that the filename matches the output of solc --version. This check now sits in validate_reported_version() in bytecode_reports_for_modified_binaries.sh. We should extract it into a separate PR check that runs on all binaries.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds to me like it's almost easier to adapt the regex to allow for prereleases as well:)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, that's an option too.
f6d6167 to
686b5a6
Compare
686b5a6 to
9d6a6c7
Compare
|
|
9d6a6c7 to
3776ee7
Compare
875a154 to
dc7b1e8
Compare
bcb67b0 to
9327b28
Compare
dc7b1e8 to
d1a3af6
Compare
9327b28 to
e4421c0
Compare
d1a3af6 to
7275366
Compare
7275366 to
1a73646
Compare
1a73646 to
fda735a
Compare
- I think we used to have such a branch in the past, but we no longer do and it being handled specially would be completely unexpected by everyone at this point.
- It is not necessary, because when missing, our CMake config will fetch it from git.
…n available instead of hard-coding "nightly"
fda735a to
8439e2a
Compare
| version = re.search('PROJECT_VERSION "([^"]+)"', f.read()).group(1) | ||
| # The full version, including alpha/beta/rc tags. | ||
| if not os.path.isfile('../prerelease.txt') or os.path.getsize('../prerelease.txt') == 0: | ||
| if os.path.isfile('../prerelease.txt') and os.path.getsize('../prerelease.txt') == 0: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
prerelease_file = '../prerelease.txt' would have been nice
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I intentionally didn't want to introduce new variables here since I'm not sure if these don't become visible as settings.
Depends on #16245.Merged.Depends on #16252.Merged.Related to #16244.
Currently our scripts do not handle tagged prereleases correctly. They assume that a tag always represents a release, at most filtering out ones not starting with
v(but not consistently). This PR adds a distinction between:vfollowed by version number onlyvfollowed by version number and-pre.suffix.develop.I also took the opportunity to remove some inconsistencies and outdated elements I spotted:
conf.pyin the docs was wrongly assuming that missingprerelease.txtmeans a release.releasebranch, which we no longer have.commit_hash.txtin CI builds.nightlyin its version, regardless of what is inprerelease.txt.The PR is incomplete, we still need to:
build_win.ps1to apply the same prerelease logic on Windows.c_release_binariesjob to run on tagged prereleases.solc-binproperly picking up prerelease binaries as such (by filling outprereleasefield inlist.json).FORCE_RELEASEbeing passed all the way down toprerelease_suffix.sh.