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

Let the nightlies call themselves 'nightly', the return. #13581

Merged
merged 1 commit into from
Nov 25, 2022

Conversation

r0qs
Copy link
Member

@r0qs r0qs commented Sep 28, 2022

Since PR#10772, nightly builds started to call themselves ci during the build.

This happens because the nightly workflow in the solc-bin repo invokes the build_emscripten.sh from the solidity repo, in line 78, after the script set the prerelease.txt to the nightly version (line 53), thus overwriting the prerelease.txt created by the workflow.

The creation of the prerelease.txt file was added to the nightly workflow here ethereum/solc-bin#64 to fix the issue when nightly builds were calling themselves develop. Now, they want to be ci, but we should not allow it!

You can see the error by running:

solc.loadRemoteVersion("v0.8.15-nightly.2022.5.27+commit.095cc647", (err, compiler) => {
    console.log(compiler.version())
})

Which returns: 0.8.15-ci.2022.5.27+commit.095cc647.Emscripten.clang
Instead of: 0.8.15-nightly.2022.5.27+commit.095cc647.Emscripten.clang

EDITED:
The PR adds a condition to not modify the prerelease.txt file if it is a nightly build.
The PR adds an optional argument to set the release binary name in the build script and also modifies the emscripten_build script to receive optional parameters for the same purpose. Parameters were used in the latter instead of arguments since the script would have more than one optional argument, requiring the user to provide the first argument even if he only wants to set the second.
The docker script was also modified to forward the parameters to the CI scripts.
If no parameters are given, the script behaves as before.

It should be merged before ethereum/solc-bin#123.

scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
@r0qs r0qs self-assigned this Oct 3, 2022
@github-actions
Copy link

This pull request is stale because it has been open for 14 days with no activity.
It will be closed in 7 days unless the stale label is removed.

@github-actions github-actions bot added the stale The issue/PR was marked as stale because it has been open for too long. label Oct 25, 2022
@cameel cameel removed the stale The issue/PR was marked as stale because it has been open for too long. label Oct 25, 2022
scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
cameel
cameel previously approved these changes Nov 9, 2022
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

I'd tweak a few more things but these are just cosmetics. In terms of functionality this now looks good.

scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
@cameel
Copy link
Member

cameel commented Nov 9, 2022

Also, needs rebase and commit squashing.

@nikola-matic
Copy link
Collaborator

Also, needs rebase and commit squashing.

Is there any difference in squashing commits manually via console, i.e. git rebase -i and simply selecting the squash and merge option on github?

@cameel
Copy link
Member

cameel commented Nov 10, 2022

Is there any difference in squashing commits manually via console, i.e. git rebase -i and simply selecting the squash and merge option on github?

Probably just description. Also, I'm not sure if it creates a merge commit (and I'd prefer if it did).

I think it's fine to use it when you know you want a single commit.

scripts/ci/build.sh Outdated Show resolved Hide resolved
scripts/ci/build.sh Outdated Show resolved Hide resolved
@cameel
Copy link
Member

cameel commented Nov 11, 2022

@r0qs I think this is still not rebased on latest develop. Note the force_release jobs that do not run. They were renamed in one of the PRs merged recently and apparently your branch is not on top of changes from that PR.

@r0qs
Copy link
Member Author

r0qs commented Nov 11, 2022

Yes. I will rebase and squash the commit. I was waiting for your input in the previous comments to do that :)

@r0qs r0qs force-pushed the ci-to-nightly branch 3 times, most recently from 369c4b0 to da80391 Compare November 12, 2022 15:01
@cameel
Copy link
Member

cameel commented Nov 22, 2022

@r0qs #13581 (comment) is the only thing left. After that it's good to merge.

cameel
cameel previously approved these changes Nov 24, 2022
Copy link
Member

@cameel cameel left a comment

Choose a reason for hiding this comment

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

Good to merge but you need to squash commits anyway so I have one extra suggestion for nicer numeric conditions.

scripts/build_emscripten.sh Outdated Show resolved Hide resolved
scripts/ci/build_emscripten.sh Outdated Show resolved Hide resolved
Co-authored-by: Kamil Śliwak <cameel2@gmail.com>
@cameel cameel merged commit 10f8142 into ethereum:develop Nov 25, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Archived in project
Development

Successfully merging this pull request may close these issues.

None yet

3 participants