Fix: official repo to list Solidity's releases #5008
Conversation
|
This PR fixes problems you can encounter if the truffle relay node is down for some reason. See this StackExchange question: https://ethereum.stackexchange.com/questions/125523/truffle-compile-list-not-showing-latest-versions-of-compiler |
|
Looks like our relay was pointing to the deprecated URLs. Fixing that now! I think for this PR, though, we want to keep relay.trufflesuite.com as the primary source, since that will allow us to fix issues without requiring a Truffle release. |
|
Thanks for bringing this to our attention! Will discuss with the team to confirm the behavior we want moving forward. |
|
Note: This is related to issue #4425. |
|
@gnidan I set that as the first repo because I found this note in the comment: this relay address exists so that we have a backup option in case more official distribution mechanisms fail, but I see your point :) |
|
This precedence and motivation, should probably be documented |
|
@cameel can you let us know which urls that Truffle should use when obtaining the solc compiler? Is |
|
My recommendation would be Overall, this should be clear from Static Binaries section in the docs. Let me know if you think it still leaves something out. |
|
Thanks @cameel! We'll make sure this gets updated in Truffle! |
|
@eggplantzzz sure, no problem, I will send an update to the PR soon. |
|
Hi @Neurone, is this PR ready for review? I see that you've made some commits, but it isn't currently passing CI. Let us know if you are able to look into what's causing this to fail. |
|
Should we keep https://solc-bin.ethereum.org/bin/ and https://ethereum.github.io/solc-bin/bin/ as backups for the backups? |
|
Hi, yes, it's ready. I didn't change anything aside from the URLs in the arrays. The error is related to the content of the loaded list: the CI tries to find a nightly build in the list ( If you agree, I can change the test here to use version0.4.16+commit.d7661dd9
|
It would not hurt. At least for |
Right. That's actually one difference between the new and old URLs - the nightlies are only available in |
I think it's the cleanest path to follow. But currently, both relays redirect to |
|
Personally, I really don't like the fact that nightlies sit in the same dir as releases since there are tons of them. If you go to the github repo backing the S3 bucket, github won't even show you all the releases because it hits the limit of 1000 files. But if you think that it's still better to have them in the new dirs, this is something we can definitely change. Could you submit an issue in the Solidity repo? |
Sure, no problem. Another solution is to add a separate file filled only with release versions. This way, we have a clean list ( |
|
For me it's only really an issue when browsing the dir manually so I don't think splitting the list would be a solution. When I do that I don't even look at the list :) |
|
I see :) I cannot list dir contents from the outside, so I didn't notice that behavior. If you don't mind having duplicated files, another option is to use the main folders for release versions only and a specific subfolder for all versions, i.e. |
|
@cameel I cannot open issues on https://github.com/ethereum/solc-bin, so I opened it on https://github.com/ethereum/solidity |
See https://github.com/ethereum/solc-bin/tree/gh-pages/bin. It's the repo backing the S3 bucket. The fact that there are so many files in there makes it kinda hard to browse that dir.
That's fine. solc-bin does not have issues exactly because we want to have them all in one place - in the main repo, Maybe we should just put that in the README. |
|
@Neurone it looks like this still isn't passing CI, we'll restart it to see if it's flaky. Otherwise, let us know if we can help resolve the issue. |
|
@kevinweaver we need to keep this PR pending until the Solidity team reaches an agreement about how to update the management of release and nightly builds (see here). When that thread is closed, I can update this PR accordingly. |
|
Maintenance note: leaving this in |
|
So it looks like However a trickier thing is that in order to build the urls we need the version commit suffix to add to the base urls we have. We get those by querying the url to see what versions are available. We need to make sure, when this gets implemented, we have a source to query for all available versions of the Solidity compiler. |
|
So additionally I found the following interesting things about these links:
|
|
Oh, looks like you're right. My mistake! Yes, that means that we have to be careful about the order of things, unless we go changing the way this works... |
|
Oh, wow, it looks like the reality is even worse than we realized -- instead of going down the list of sources to find a working source, and then using that source for both the version list and the compiler itself, it goes down the list of sources to find a working source for the version list, uses that to get the filename for the compiler, and then goes down the list again to find the compiler, even though filenames may differ between sources! Yikes! This whole process needs to be reworked to be actually sensible... |
|
You can find work in progress to fix Truffle's retry mechanism for downloading the Soldity compiler lists and the compiler itself here. |
Added the official repo for Solidity's Compiler releases, before the Truffle custom one. Added a note for the last two deprecated repo.
|
Hey @Neurone, that PR mentioned in my previous comment has been merged and released so I believe we are ready to try and get this PR through. It will require a little bit more work, however. Can you give me permission to your repo so that I can rebase it and perform the final work? If you would rather do this then let me know and we can work on getting this through. Thanks! |
|
@eggplantzzz sure! This would be very helpful also because I forgot almost everything about this PR, and you can do a better job for sure :D I just sent you an invitation. |
|
Thanks @Neurone for doing this work! |
eggplantzzz
requested review from
haltman-at,
gnidan,
cds-amal,
cliffoo,
dongmingh,
fainashalts,
lsqproduction and
sukanyaparashar
|
|
||
| // Delete if it's already there. | ||
| if (await fse.exists(expectedCache)) await fse.unlink(expectedCache); | ||
| if (await fse.exists(compilerCacheDirectory)) |
There was a problem hiding this comment.
Minor note here: fs.exists is deprecated. You should probably use fs.existsSync instead.
There was a problem hiding this comment.
I'll go ahead and address these concerns in #5445 as it moves the compile-solidity tests to a new package. It should make dealing with merge conflict stuff easier.
There was a problem hiding this comment.
Oh actually I will need to address these here I think since some of these are related to necessary test suite changes for this particular PR.
|
|
||
| // Delete if it's already there. | ||
| if (await fse.exists(expectedCache)) await fse.unlink(expectedCache); | ||
| if (await fse.exists(compilerCacheDirectory)) | ||
| await fse.removeSync(compilerCacheDirectory); |
There was a problem hiding this comment.
Style note: You should probably put braces around the body of this block, indented blocks without braces can be a source of confusion for later editors.
Also, since you're using removeSync, the await is redundant.
| assert(await fse.exists(expectedCache), "Should have cached compiler"); | ||
| const cachedCompilerFilenames = fse.readdirSync(compilerCacheDirectory); | ||
| const compilerFilename = cachedCompilerFilenames.find(filename => { | ||
| return filename.includes("v0.4.21+commit.dfe3193c"); |
There was a problem hiding this comment.
This is truly nitpicking and you don't need to change this, but filename => { return foo; } could instead just be filename => foo.
| return filename.includes("v0.4.21+commit.dfe3193c"); | ||
| }); | ||
|
|
||
| assert( |
There was a problem hiding this comment.
Another style note -- it's better to use assert.somethingMoreSpecific when it exists; in this case, that could be assert.notEqual or assert.notStrictEqual.
| const cachedCompilersFilenames = fse.readdirSync(compilersCacheDirectory); | ||
|
|
||
| assert( | ||
| cachedCompilersFilenames.some(filename => { |
There was a problem hiding this comment.
Same note here as earlier about filename => { return foo; }.
| return filename.includes("v0.4.21+commit.dfe3193c"); | ||
| }); | ||
|
|
||
| assert.equal( |
There was a problem hiding this comment.
(Hey @eggplantzzz looks like you inverted this one)
There was a problem hiding this comment.
oh I did this algorithmically and didn't think about the value - isUndefined should be used
There was a problem hiding this comment.
oh no that's also wrong, it should be defined :P
There was a problem hiding this comment.
Well, you're just using assert and not chai here, right? Node's builtin assert doesn't have isDefined, that's a chai thing...
There was a problem hiding this comment.
oh I thought it was using chai...easily remedied
|
Thanks again @Neurone! |
Added the official repo for Solidity's Compiler releases, before the Truffle custom one.
Added a note for the last two deprecated repo.