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

Github action for emscripten nightly builds #30

Merged
merged 1 commit into from
Jul 23, 2020

Conversation

cameel
Copy link
Member

@cameel cameel commented Jul 3, 2020

Part of ethereum/solidity#9258. Replaces #29.

Another go at the same problem. This time doing a full checkout and building soljson.js within the action instead of pulling it from CircleCI. This does make things simpler and pretty much solves all the outstanding problems from the previous PR. I just still need answers for the two questions below.

It's also significantly slower now: ~20 min for the build, ~3.5 min for git checkout, ~8 min for the update script.

I went with @ekpyron's suggestion and split the workflow into two jobs with soljson.js being passed between them as an artifact. This way we can still reuse the second part (that updates solc-bin) if we ever decide to go back to the solution with pulling from CircleCI.

Questions

  • I need an e-mail address to put in the committer field (currently it's emscripten nightly action <solidity@example.com>).
  • I have created a master branch in the repo and set it as the branch where the action adds new commits. Since there are external tools that rely on binaries hosted on GH pages, I think that it would be best to consider gh-pages frozen until we get S3 ready to serve the files instead and to work on master in the meantime.
    • After discussing it on Gitter, I'm dropping the master branch. We'll keep updating gh-pages and just exclude files that go over the GH pages limit using jekyll config.

@cameel cameel requested review from ekpyron, aarlt and chriseth July 3, 2020 21:36
@cameel cameel self-assigned this Jul 3, 2020
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 19b938d to 7c802cd Compare July 3, 2020 21:36
@cameel cameel marked this pull request as draft July 3, 2020 21:36
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 7c802cd to a423a97 Compare July 3, 2020 21:40
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch 2 times, most recently from 9428fd9 to 77d294d Compare July 4, 2020 00:05
@cameel cameel changed the base branch from gh-pages to master July 4, 2020 00:06
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 77d294d to 4810994 Compare July 4, 2020 00:10
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 4810994 to ac0f148 Compare July 7, 2020 10:14
@cameel cameel changed the base branch from master to gh-pages July 7, 2020 10:18
@cameel
Copy link
Member Author

cameel commented Jul 7, 2020

Changing target branch to gh-pages.

I have also rebased it on top of it. Still working on moving parts of the action into a script.

@cameel cameel force-pushed the github-action-emscripten-nightly-build branch 2 times, most recently from 409fd2f to d9f3654 Compare July 7, 2020 10:43
.circleci/config.yml Outdated Show resolved Hide resolved
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch 4 times, most recently from 43ce483 to 298e040 Compare July 8, 2020 17:35
@cameel cameel changed the title [WIP] Github action for emscripten nightly builds Github action for emscripten nightly builds Jul 8, 2020
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 298e040 to 153c763 Compare July 8, 2020 17:55
@cameel cameel marked this pull request as ready for review July 8, 2020 18:21
@ekpyron
Copy link
Member

ekpyron commented Jul 8, 2020

The current test failure in this PR here, is something we have seen a lot and I still don't really understand.
There's some days with multiple "nightlies" that day - apparently depending on who runs the script the sorting according to the commit hashes is different... The CI seems to order inverse-lexicographically, descending letters first, then numbers... which is as good as any order and the same that I always got locally... but other systems seem to produce another order (which always seemed rather random to me)...

@cameel
Copy link
Member Author

cameel commented Jul 8, 2020

@ekpyron I checked semver spec and looks like it does not specify ordering for metadata (the part after the plus sign):

Semantic Versioning 2.0.0 > bullet point 10

Build metadata MUST be ignored when determining version precedence. Thus two versions that differ only in the build metadata, have the same precedence. Examples: 1.0.0-alpha+001, 1.0.0+20130313144700, 1.0.0-beta+exp.sha.5114f85, 1.0.0+21AF26D3—-117B344092BD.

So e.g. 0.4.2-nightly.2016.9.17+commit.212e0160 is treated as equal to v0.4.2-nightly.2016.9.17+commit.a78e7794. We have to add an extra commit hash comparison if we want the order to be stable.

But the weird part is - why do we have several nightlies from 2016-09-17? Deleting all but one would solve the problem too :)

@chriseth
Copy link
Contributor

chriseth commented Jul 9, 2020

There should only be one nightly per day.

@ekpyron
Copy link
Member

ekpyron commented Jul 9, 2020

There should only be one nightly per day.

Sure there should, but what do we do with the old ones for which this isn't the case :-)?

What's the most recent one for which there are more than one? If it's 0.4.2, as the CI run indicates, then we could just remove all nightlies older than 0.4.3 as well (I chose how many to already remove from solc-bin quite arbitrarily anyways, so little harm in removing a few more, is there?).

@cameel
Copy link
Member Author

cameel commented Jul 9, 2020

we could just remove all nightlies older than 0.4.3 as well

Do you mean permanently? The nightlies that were already removed are going to be restored soon and just excluded from jekyll config. So that's a bit different from removing the "duplicates".

@ekpyron
Copy link
Member

ekpyron commented Jul 9, 2020

we could just remove all nightlies older than 0.4.3 as well

Do you mean permanently? The nightlies that were already removed are going to be restored soon and just excluded from jekyll config. So that's a bit different from removing the "duplicates".

Ok, that's a fair point then... But on what basis would you choose one to keep and delete others? It's not "duplicates" if it's different hashes - it's different versions we did publish once... what justifies keeping one and not another?

Also: if we exclude them from the jekyll config, then we also have to exclude them from the lists, don't we? It would be weird, if the lists talk about files that don't exist... and that would be equivalent to removing them entirely as far as this particular issue is concerned...

@cameel
Copy link
Member Author

cameel commented Jul 9, 2020

Yeah, I know they're not identical. That's why I put it in quotes. I really meant that all but one are redundant. I don't think any one of them is more important than the others so it does not really matter which one we keep.

Also: if we exclude them from the jekyll config, then we also have to exclude them from the lists, don't we?

Right. That's still an open question. I think I asked about it somewhere before (on Gitter?) but it got lost in the discussion.

The list is indeed a problem. I think we should remove the files from the list but then we'd have to somehow serve different lists from GH pages. And that's not the only problem. soljson.js is now a symlink and soljson-latest.js and soljson-nightly.js in bin/ will be too (after #33). They will keep working because jekyll replaces the link with a copy but now that copy won't match the latest visible release/nightly.

Maybe we could somehow add a second set of lists and switch them over during rendering to GH pages. And change symlinks on the fly during rendring too - but it's getting complicated quickly. Freezing the branch seems like a much simpler solution so that's what I would suggest instead.

@ekpyron
Copy link
Member

ekpyron commented Jul 9, 2020

Quote from gitter:

@ekpyron: @cameel @chriseth Maybe we shouldn't complicate ethereum/solc-bin#30 just because of ancient nightlies :-)... what's the best way to resolve that quickly? No-one will ever really need those old 0.4.2 nightlies...
@chriseth yes, just delete them

I agree with that - just get rid of those old nightlies altogether and the problem is solved. We also don't need to bring the ones already deleted back.
EDIT: at least for now, so that this doesn't stall things. If the need ever arrises, we can still think about it, but I think it highly unlikely that anyone will ever need a 0.4.2 nightly...

@chriseth
Copy link
Contributor

chriseth commented Jul 9, 2020

Clarification: I was talking about the duplicate nightlies as in #30 (comment)

Copy link
Member

@aarlt aarlt left a comment

Choose a reason for hiding this comment

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

I was wondering whether the nightlies should not also get tested before publishing. There seem to be a script for testing emscripten ./scripts/test_emscripten.sh. It also is referenced in .travis.yml.

@cameel
Copy link
Member Author

cameel commented Jul 13, 2020

@aarlt Right. They probably should. I'll get back to this and add the check when I'm done with the other PRs.

@ekpyron
Copy link
Member

ekpyron commented Jul 13, 2020

Yeah, I was wondering whether to suggest running emscripten tests as well.
We could, but one could argue either way. If we had fetched the nightlies from CircleCI we wouldn't have tested them either - and no-one should actually use the nightlies for any production use anyways.

It wouldn't be too hard in any case - we can just spawn a job in between the two existing ones - you should be able to pretty much take over the test_emscripten job from https://github.com/ekpyron/solidity/blob/5a9345798e396e79d18faed61ed1de5c094557ef/.github/workflows/emscripten.yml - you'd just need to change the checkout URL for the solidity checkout (and pass over the version, resp. re-execute the get_version script).
We could think about just having it in there (it failing should then send out a mail to us I think) - or whether to make it an actual requirement for publishing the nightlies.

@cameel
Copy link
Member Author

cameel commented Jul 13, 2020

Thanks. I'll use that.

By the way, if you think this check is not all that essential then I can also do it in a separate PR not to block this one.

@ekpyron
Copy link
Member

ekpyron commented Jul 13, 2020

Separate PR seems fine to me for it!

@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 69f2b6a to a2bace0 Compare July 16, 2020 23:43
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from a2bace0 to dc173ba Compare July 17, 2020 22:57
@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from dc173ba to 785dfa9 Compare July 21, 2020 14:42
@cameel
Copy link
Member Author

cameel commented Jul 21, 2020

So looks like this is ready to get merged.

I didn't get an answer about license and keeping the scripts in solc-bin so I assume it's OK as is.

@cameel cameel force-pushed the github-action-emscripten-nightly-build branch from 785dfa9 to a4a0f89 Compare July 23, 2020 10:40
@chriseth chriseth merged commit 70d5138 into gh-pages Jul 23, 2020
@cameel cameel deleted the github-action-emscripten-nightly-build branch July 23, 2020 12:29
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.

4 participants