-
Notifications
You must be signed in to change notification settings - Fork 409
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
Script for downloading release binaries from github #41
Conversation
29c2692
to
e8565ae
Compare
e8565ae
to
ca4220f
Compare
Moving back to draft since after the partial revert of #39 it does require some tweaks. |
@@ -0,0 +1,287 @@ | |||
#!/usr/bin/env bash |
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.
Random comment: do you want to enable shellcheck
on this repo prior to this commit?
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 just ran into this comment randomly, sorry for not replying earlier.
This is actually a good reason to move scripts I originally put into this repo to the main one. I'm planning to do that eventually. I already did that with bytecode PR check.
Not sure about this particular script though. I'd like to salvage it if possible but currently I'm not sure if we actually have a use for it. We should probably talk about it on one of the planning meetings but this currently isn't too high on the priority list for me.
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.
Not sure it makes sense moving the s3-sync.sh
to another repo, so perhaps better to have shellcheck here in any case.
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, I think that even s3-sync.sh
would fit right in with other scripts that are already in https://github.com/ethereum/solidity/tree/develop/scripts/solc-bin. I even think it might make sense to move update
there. solc-bin
with it's current size is not very convenient to use for coding and after doing that for some time I'm more and more convinced that it should contain only binaries and nothing else.
One downside of having code in this repo is that accessing it requires getting a checkout, which is big and wastes a lot of time. This is sometimes an obstacle when you want to have multliple jobs in a single workflow and not all of them need to access binaries. It's not that hard to work around - you can pass the scripts as artifacts or download individual files via github API - but it's still a bit annoying when you need to do this.
To be honest I'm not strongly convinced that we must have everything in the main repo - I see both upsides and downsides - I'm just currently leaning more towards that option. Having shellcheck here would not be bad either.
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.
Also, it would be nice to have some of these scripts rewritten in Python eventually and for that we already have nice linting and testing infrastructure in main repo. For example the bytecode comparison check for PRs is written in Bash and this is the main reason why I gave up on running it on Windows binaries (they're being ignored currently). Originally I wanted to support them but I realized that I'd have to write a separate PowerShell or CMD version of the scripts just for that one platform.
The script here is now a bit out of date (our artifacts changed, CircleCI has released API v2.0, etc.). I'm going to close this in favor of ethereum/solidity#12181, which reuses parts of this script. |
Depends on on #39. Part of ethereum/solidity#9258.
This is the part of #35 that only deals with github. I used the script to get old releases from github and I'm also going to hook it up as a PR check to compare newly added releases with those on the release page.
The script:
soljson
and puts it in the right directory.update
script needs to be run separately (that was simpler than adding a flag to disable the update).soljson.js
from v0.1.3. I could have added a special case for it but decided that it was not worth the effort. I downloaded that release manually.