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

Include github URLs in list.json #19

Open
wants to merge 4 commits into
base: gh-pages
Choose a base branch
from
Open

Include github URLs in list.json #19

wants to merge 4 commits into from

Conversation

axic
Copy link
Member

@axic axic commented Mar 28, 2018

After this change it is possible to rely only on list.json in a client. The client can look for http:// or https:// depending on what they run on.

@axic axic requested a review from chriseth March 28, 2018 11:56
pars.urls = [
'bzzr://' + swarmhash(fileContent).toString('hex'),
'https://ethereum.github.io/solc-bin/bin/' + pars.path,
'http://ethereum.github.io/solc-bin/bin' + pars.path
Copy link
Member Author

Choose a reason for hiding this comment

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

Actually since we have solc-bin.ethereum.org as a redirect there, should we use that?

Copy link
Contributor

Choose a reason for hiding this comment

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

Why do we need absolute links here? This file is just a summary of the other files in the same location.

Copy link
Member Author

Choose a reason for hiding this comment

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

It would work if we consider the current approach with github pages, however I was thinking about having the "manifest" (list.json) available on swarm or ipfs and as such it would need absolute URLs.

We could include a relative path as an option in the URLs though.

Copy link
Contributor

Choose a reason for hiding this comment

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

Actually if you use a swarm manifest, that would be an argument to use relative paths (which are then valid both inside swarm and inside github pages).

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually if you use a swarm manifest, that would be an argument to use relative paths (which are then valid both inside swarm and inside github pages).

I am not entirely sure how that would work with Swarm manifests and whether we want to keep this /bin directory at all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh btw it is a URL and not a path. Not sure how to represent relative paths on URLs.

Copy link
Contributor

Choose a reason for hiding this comment

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

We already have the path member, so actually no need to put them here at all.
We could add ipfs urls, though...

Copy link
Member Author

Choose a reason for hiding this comment

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

I think we actually want to add the official https://binaries.soliditylang.org/... URL.

Reason: all the URLs we have in this array are pointing to a specific file and are not paths within a manifest/collection. If we publish list.json + the paths as a collection as well, for that use case the consumer can try the path, otherwise if list.json is just independently published on IPFS or swarm they can still opt to acquire the files via various means.

@axic
Copy link
Member Author

axic commented Apr 2, 2018

ping @chriseth

@cameel
Copy link
Member

cameel commented Jul 3, 2020

@axic Maybe, instead of adding new URL variants to the list in every dict we should rather add a separate, top-level key with official sources? The number of sources will probably keep getting bigger. We already want to add IPFS so that'll be 4 items per file. The dicts already contain enough information to construct URLs if you know the source.

Also, I think that having a list of official sources (either in the file, or maybe just in README) would have helped us with the transition to S3. solc-bin.ethereum.org will keep working after we switch to S3 for hosting but we want to disable GH pages (i.e. ethereum.github.io). This will break tools that depend on the latter. They shouldn't depend on it but I don't know if it has been communicated clearly enough so they might. If there was a list, it would have been more obvious.

@chriseth
Copy link
Contributor

chriseth commented Jul 6, 2020

I would prefer it if this is really an index of the current list - and thus does not depend on or contain any information about where it is located.

@cameel
Copy link
Member

cameel commented Jul 6, 2020

How about including a separate, static file in the root of the repository, with a list of possible source URLs?

"bzzr://8f3c028825a1b72645f46920b67dca9432a87fc37a8940a2b2ce1dd6ddc2e29b"
"bzzr://8f3c028825a1b72645f46920b67dca9432a87fc37a8940a2b2ce1dd6ddc2e29b",
"https://ethereum.github.io/solc-bin/bin/soljson-v0.1.1+commit.6ff4cd6.js",
"http://ethereum.github.io/solc-bin/binsoljson-v0.1.1+commit.6ff4cd6.js"
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't this be

Suggested change
"http://ethereum.github.io/solc-bin/binsoljson-v0.1.1+commit.6ff4cd6.js"
"http://ethereum.github.io/solc-bin/bin/soljson-v0.1.1+commit.6ff4cd6.js"

?

This comment was marked as spam.

Copy link

@Varshans2 Varshans2 left a comment

Choose a reason for hiding this comment

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

good

@cameel cameel requested a review from r0qs November 22, 2022 15:28
@r0qs
Copy link
Member

r0qs commented Dec 1, 2022

@axic @cameel should we close this? And instead work in the other suggested approaches here: #19 (comment) and here: ethereum/solidity#2238 (comment)

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.

9 participants