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.

Sign up for GitHub

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

Jump to bottom

Use GitHub raw.githubusercontent.com to avoid rate limiting #446

Merged
merged 1 commit into from
Feb 18, 2020
Merged

Use GitHub raw.githubusercontent.com to avoid rate limiting #446

merged 1 commit into from
Feb 18, 2020

Conversation

jleeh
Copy link
Contributor

@jleeh jleeh commented Feb 18, 2020 •
edited
Loading

Fixes #445.

  • Rate limit is being hit causing flakey builds for anyone who depends on solc-js remote downloading in their software. The change to GitHub raw URLs avoids hitting rate limits.
  • Ability to override the repository all together with an environment variable SOLC_REPO. Allowing people to use alternative repos, including internal.

@coveralls
Copy link

coveralls commented Feb 18, 2020 •
edited
Loading

Coverage Status

Coverage remained the same at 88.651% when pulling 9309f5a on smartcontractkit:enhancement/solc-repo into 3ec5731 on ethereum:master.

chriseth
chriseth reviewed Feb 18, 2020
View reviewed changes
downloadCurrentVersion.js Outdated
@@ -13,7 +15,7 @@ function getVersionList (cb) {
console.log('Retrieving available version list...');

var mem = new MemoryStream(null, { readable: false });
https.get('https://ethereum.github.io/solc-bin/bin/list.json', function (response) {
https.get(SOLC_REPO + 'list.json', function (response) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
https.get(SOLC_REPO + 'list.json', function (response) {
https.get(SOLC_REPO + '/list.json', function (response) {

Just in case people forget a trailing slash in the env variable?

@chriseth
Copy link
Contributor

Can you document this feature in the readme?

@edisinovcic does this conflict with your work?

@edisinovcic
Copy link

No, it's definitely a welcomed PR.

axic
axic reviewed Feb 18, 2020
View reviewed changes
downloadCurrentVersion.js Outdated
@@ -3,6 +3,8 @@
// This is used to download the correct binary version
// as part of the prepublish step.

const SOLC_REPO = process.env.SOLC_REPO || 'https://raw.githubusercontent.com/ethereum/solc-bin/gh-pages/bin/';
Copy link
Member

Choose a reason for hiding this comment

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

What is the use case for changing the official URL here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The GitHub pages URL becomes rate limited once you exceed certain bandwidth allowance, which made our CI builds fail when downloading solc. Using the raw URL bypasses the pages rate limits.

Copy link
Member

Choose a reason for hiding this comment

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

I understand that, but not the reason for env.SOLC_REPO.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Commented underneath.

@axic
Copy link
Member

axic commented Feb 18, 2020

Changing it here, while keeping the old URL in wrapper.js creates an inconsistency in the library.

@edisinovcic
Copy link

Definitely @axic but I can update it for the whole project for it to be configurable if that's okay with you?

@axic
Copy link
Member

axic commented Feb 18, 2020

for the whole project for it to be configurable

I am not yet seeing the use case for that. Also note that wrapper.js should definitely not use environment variables as it is a library and not CLI.

@samsondav samsondav mentioned this pull request Feb 18, 2020
Closed
@jleeh
Copy link
Contributor Author

jleeh commented Feb 18, 2020 •
edited
Loading

I am not yet seeing the use case for that.

Mostly for pointing to internal repositories/elsewhere. Even though there's checksums, some teams could want to lock versions in their own repository, or remove the dependency on GitHub all together.

I'll update the URL in wrapper.js, but in terms of adding an override since it's a lib, I feel I wouldn't be best making that change.

@axic
Copy link
Member

axic commented Feb 18, 2020

I'd prefer we merge this without the override (to include it in 0.6.3) and then we can discuss the override separately and land it in 0.6.4. Does that sounds like a good middle gorund?

@axic axic changed the title Use GitHub raw, ability to override when remote downloading Use GitHub raw.githubusercontent.com to avoid rate limiting Feb 18, 2020
@axic
Copy link
Member

axic commented Feb 18, 2020

@jleeh can you create a new PR for the override?

@jleeh
Copy link
Contributor Author

jleeh commented Feb 18, 2020

Sure. I'll base it on this branch to avoid future conflicts.

@axic axic merged commit 40e3844 into ethereum:master Feb 18, 2020
@HenryNguyen5 HenryNguyen5 mentioned this pull request Mar 23, 2020
Closed
@HenryNguyen5 HenryNguyen5 mentioned this pull request Apr 21, 2020
Closed
@cameel cameel mentioned this pull request Jul 20, 2020
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@chriseth chriseth chriseth left review comments

@axic axic axic approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Install often times out due to github pages rate limit
5 participants
@jleeh @coveralls @chriseth @edisinovcic @axic