added commit-hash.txt file under export-subst flag by nishant-sachdeva · Pull Request #12717 · argotorg/solidity

nishant-sachdeva · 2022-02-25T18:09:48Z

chriseth · 2022-02-28T10:58:19Z

Approach looks good. We need to change all the code that reads those files and query the information from git if the contents contain placeholders.

chriseth · 2022-02-28T10:58:47Z

Maybe we can even execute a git command to do the replacement (of course not modify the files)?

cameel · 2022-03-01T11:21:31Z

Not sure if job failures are even related to the PR. There were some CircleCI problems yesterday to I reran it to make sure. It looks like CircleCI had some problems executing the bash code in the config. If it's reproducible, could also be that they changed something in their API/config format but that seems unlikely to me.

cameel · 2022-03-01T11:25:27Z

Approach looks good. We need to change all the code that reads those files and query the information from git if the contents contain placeholders.

@nishant-sachdeva Does this replace the placeholders only when you explicitly run git archive or every time the code is checked out (would be nice to actually have a comment explaining that in the code)? If it's the former, you'll need to look through everything that reads these two files and manually tell either git to generate them before hand or just treat it as if the files did not exist.

ekpyron · 2022-03-01T12:24:58Z

It'll only replace the placeholders on git archive, so yes: we will need to change the logic when reading those files, in particular cmake/scripts/buildinfo.cmake.

If the files do not contain placeholders, we can assume that it's a release tarball and use the commit hash from the file.

The tricky part will be the case, in which the files do contain placeholders: we will probably need to check if there is a release tag at the current commit to be able to tell, if we should build as prerelease or as release. EDIT: ah right - or we can inspect the top of the changelog instead as suggested in #9720.

ekpyron · 2022-03-01T12:31:35Z

Using $Format:%d$ is probably too error-prone.
I think we may need to replace the prerelease.txt mechanism entirely and instead have something like a date.txt file plus checking the changelog for unreleased (unless someone comes up with something better).

cameel · 2022-03-01T13:11:44Z

Is there are way for git to create the files during git archive? There would be fewer changes needed if they did not exist by default. Or they could exist but under a different name, like prerelease.txt.template or something.

ekpyron · 2022-03-01T13:27:48Z

Is there are way for git to create the files during git archive? There would be fewer changes needed if they did not exist by default. Or they could exist but under a different name, like prerelease.txt.template or something.

Would that really help that much? In any case, I think the only thing you can do is to exclude files from git archive, which is not much help for our case.

The only complicated part I really see for this is to determine whether we're at a release tag on a git checkout.

ekpyron · 2022-03-01T13:29:23Z

Or put differently: for the commit hash this is straightforward, but the prerelease/release logic we will basically have to rewrite for this no matter what.

cameel · 2022-03-03T12:15:28Z

Would that really help that much?

I think it would. As I see it, the issue is all about source archives created by github. If these files were created only on git archive, we'd have proper source archives and keep everything working as it used to in other cases. We'd not have to worry that the files may exist but still contain the template. So we'd be done here already.

cmake/scripts/buildinfo.cmake

ekpyron · 2022-03-03T14:45:46Z

cmake/scripts/buildinfo.cmake

@@ -32,15 +32,19 @@ endif()
 if (EXISTS ${ETH_SOURCE_DIR}/commit_hash.txt)


We should really make it an error if it doesn't exist instead of just doing this conditionally.
Does the file(READ thing cause an error if the file doesn't exist? If so we can just remove the ìf entirely.

Why was this marked as resolved?

cmake/scripts/buildinfo.cmake

docs/conf.py

.circleci/build_win.ps1

.gitattributes

CMakeLists.txt

cmake/current_tag.txt

ekpyron · 2022-04-04T11:10:12Z

cmake/scripts/buildinfo.cmake

+		)
+	endif()


We could also query git tag --points-at HEAD and check if it's a v* tag...
Ideally, we would default SOL_FORCE_RELEASE to true in that case, but that may be a hassle, so we could also just always force a release build then...

ekpyron · 2022-04-04T11:11:14Z

scripts/build.sh

 cd "${BUILDDIR}"

-cmake .. -DCMAKE_BUILD_TYPE="$BUILD_TYPE" "${@:2}"
+if [[ "$(git tag --points-at HEAD 2>/dev/null)" == v* ]]; then


Is this robust in the case there are several tags at HEAD?
That usually won't happen, but if it ever does, this might break...
It should really check, whether any tag at the HEAD is a v* tag, shouldn't it?

ekpyron · 2022-04-05T10:52:50Z

.circleci/build_win.ps1

 if ("$Env:FORCE_RELEASE" -Or "$Env:CIRCLE_TAG") {
-	New-Item prerelease.txt -type file
 	Write-Host "Building release version."
+	..\deps\cmake\bin\cmake --build . -j 10 --target install --config Release -DSOL_RELEASE_VERSION="On"


The cmake options here and below don't seem to match the new cmake options defined below, do they?
Also it doesn't seem nice to invoke cmake here and then again below...

CMakeLists.txt

cmake/scripts/buildinfo.cmake

ekpyron · 2022-08-10T16:56:08Z

I think it's easier to redo this properly at some point rather than keeping this PR alive, so I'm closing for now.

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 4 times, most recently from e93e202 to 17395cd Compare February 26, 2022 15:03

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 4 times, most recently from be09f5c to ee4005f Compare February 28, 2022 18:13

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from ee4005f to 8ace852 Compare March 3, 2022 14:21

ekpyron reviewed Mar 3, 2022

View reviewed changes

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

ekpyron reviewed Mar 3, 2022

View reviewed changes

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from 8ace852 to 4bd0405 Compare March 3, 2022 14:35

ekpyron reviewed Mar 3, 2022

View reviewed changes

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 5 times, most recently from 92b0664 to aecdd93 Compare March 3, 2022 15:14

cameel requested changes Mar 3, 2022

View reviewed changes

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from aecdd93 to a04d83f Compare March 4, 2022 14:26

aarlt reviewed Mar 24, 2022

View reviewed changes

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

aarlt reviewed Mar 24, 2022

View reviewed changes

docs/conf.py Show resolved Hide resolved

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 9 times, most recently from 5bace78 to 8433936 Compare April 1, 2022 10:21

cameel reviewed Apr 1, 2022

View reviewed changes

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 3 times, most recently from 7c4552f to 75291ac Compare April 4, 2022 10:06

ekpyron reviewed Apr 4, 2022

View reviewed changes

cmake/current_tag.txt Outdated Show resolved Hide resolved

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from 75291ac to adaa92d Compare April 4, 2022 10:56

ekpyron reviewed Apr 4, 2022

View reviewed changes

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from adaa92d to 1294836 Compare April 4, 2022 11:23

ekpyron reviewed Apr 5, 2022

View reviewed changes

CMakeLists.txt Outdated Show resolved Hide resolved

ekpyron reviewed Apr 5, 2022

View reviewed changes

cmake/scripts/buildinfo.cmake Outdated Show resolved Hide resolved

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch 2 times, most recently from 6c68d8b to ec10d35 Compare April 7, 2022 05:10

Added commit_hash.txt and prerelease.txt

5feb9a9

nishant-sachdeva force-pushed the replacing_commit_hash_and_prerelease_with_git_archive_placeholders branch from ec10d35 to 5feb9a9 Compare April 7, 2022 05:14

ekpyron self-assigned this Apr 7, 2022

ekpyron closed this Aug 10, 2022

		@@ -32,15 +32,19 @@ endif()
		if (EXISTS ${ETH_SOURCE_DIR}/commit_hash.txt)

Conversation

nishant-sachdeva commented Feb 25, 2022

Uh oh!

chriseth commented Feb 28, 2022

Uh oh!

chriseth commented Feb 28, 2022

Uh oh!

cameel commented Mar 1, 2022

Uh oh!

cameel commented Mar 1, 2022

Uh oh!

ekpyron commented Mar 1, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ekpyron commented Mar 1, 2022

Uh oh!

cameel commented Mar 1, 2022

Uh oh!

ekpyron commented Mar 1, 2022

Uh oh!

ekpyron commented Mar 1, 2022

Uh oh!

cameel commented Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekpyron Mar 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ekpyron Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ekpyron Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

ekpyron Apr 4, 2022

Choose a reason for hiding this comment

Uh oh!

ekpyron Apr 5, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

ekpyron commented Aug 10, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

Comments

ekpyron commented Mar 1, 2022 •

edited

Loading

cameel commented Mar 3, 2022 •

edited

Loading

ekpyron Mar 3, 2022 •

edited

Loading