-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Implement robustness improvements in the compiler downloader #6056
Conversation
|
The latest updates on your projects. Learn more about Vercel for Git ↗︎
|
a2c8b5c
to
70d8632
Compare
70d8632
to
c7b302e
Compare
c7b302e
to
d7ad1d3
Compare
d7ad1d3
to
50e983f
Compare
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 have copied the multi-process mutex from the v2.
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 have copied the compiler downloader tests from v2 and updated them to work with the v3 implementation.
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 have copied the compiler tests from v2 and updated them to work with the v3 implementation. One significant update is the installation of tsx for the solcjs compiler tests.
assertRejectsWithHardhatError( | ||
() => mockDownloader.downloadCompiler("0.4.12"), | ||
HardhatError.ERRORS.SOLIDITY.INVALID_DOWNLOAD, | ||
{ | ||
remoteVersion: "0.4.12+commit.194ff033", | ||
}, | ||
); |
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've seen this check being flaky already as we sometimes get HardhatError.ERRORS.SOLIDITY.DOWNLOAD_FAILED
instead. What do you think about adding a retry mechanism to the downloader?
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 like the idea, but let's discuss it with @alcuadrado. Either way, this should be addressed in a separate PR.
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 had another look at that and I don't quite understand why this is being flaky. We're using a mock downloader in this test case, which just writes to the disk. I assumed it was flaky because of the network use, but we're not even touching it here.
I'm trying to capture the full error we get when this flakes on another branch. I'll report back here when I find something.
I still think a retry mechanism might be an idea worth exploring, but I would definitely like to understand the source of flakiness in this test case better.
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.
Found it! It was throwing the dir doesn't exist error because we weren't awaiting the assert 🤦 We should be able to catch it with a linting rule.
assertRejectsWithHardhatError( | |
() => mockDownloader.downloadCompiler("0.4.12"), | |
HardhatError.ERRORS.SOLIDITY.INVALID_DOWNLOAD, | |
{ | |
remoteVersion: "0.4.12+commit.194ff033", | |
}, | |
); | |
await assertRejectsWithHardhatError( | |
() => mockDownloader.downloadCompiler("0.4.12"), | |
HardhatError.ERRORS.SOLIDITY.INVALID_DOWNLOAD, | |
{ | |
remoteVersion: "0.4.12+commit.194ff033", | |
}, | |
); |
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.
Hey Piotr! Sorry for the delay in reviewing this. It looks good. I just left a few comments.
v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compiler/downloader.ts
Outdated
Show resolved
Hide resolved
v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compiler/downloader.ts
Show resolved
Hide resolved
v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compiler/downloader.ts
Show resolved
Hide resolved
assertRejectsWithHardhatError( | ||
() => mockDownloader.downloadCompiler("0.4.12"), | ||
HardhatError.ERRORS.SOLIDITY.INVALID_DOWNLOAD, | ||
{ | ||
remoteVersion: "0.4.12+commit.194ff033", | ||
}, | ||
); |
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 like the idea, but let's discuss it with @alcuadrado. Either way, this should be addressed in a separate PR.
v-next/hardhat/test/internal/builtin-plugins/solidity/build-system/compiler/index.ts
Show resolved
Hide resolved
hardhatTotal size of the bundle: List of dependencies (sorted by size)
|
Resolves #5990
This PR implements changes outlined in #5990. Please note, that trimming down the solc dependency has been extracted to a separate issue #6071 as it will be easier to review that way.