Increase maxBuffer to 10MB when compiling using native compiler #3002
Conversation
gnidan
left a comment
gnidan
left a comment
There was a problem hiding this comment.
Apart from the quote style change, this looks good!
packages/compile-solidity/compilerSupplier/loadingStrategies/Native.js
Outdated
Show resolved
Hide resolved
|
If you undo the quote style changes, I'll take a stab at fixing eslint |
|
Thank you for this btw! |
danhper
force-pushed
the
increase-native-buffer-size
branch
from
020e0a9 to
d6a90cd
Compare
|
Thanks for the quick reply. I removed all the style changes. |
|
Cool, thank you! Hopefully we'll have the build fixed today so we can merge this! |
|
Tests seem to be passing so hopefully it should be good to merge? |
| }; | ||
| } catch (error) { | ||
| if (error.message === "No matching version found") { | ||
| throw this.errors("noVersion", versionString); |
There was a problem hiding this comment.
Why'd you remove this invocation? Don't we need to handle this case? (@eggplantzzz do you know?)
There was a problem hiding this comment.
Unless I am missing something, this is returning an object and not executing anything, so there is no way that it will raise an exception at this point.
compile might fail later on but having the try/catch here will not help.
Simplified example:
var a;
// try/catch not helping
try {
a = () => { thrown RuntimeError('asdf') }
} catch (e) { console.log(e) }
a() // will not be caughtTBH I changed this mainly to please eslint so if you think it's better to keep this for another PR I can of course revert this change.
There was a problem hiding this comment.
Hm, I prefer leaving code alone unless the change is warranted. @eggplantzzz added that line and I suspect it does something important.
It'll definitely make this an easy merge at this point if you restrict this PR to just the max buffer change. Happy to figure out the error handling business in an issue or a subsequent PR (you could very well be right, but I just don't want to keep you waiting on this particular issue... would love to get your fix here out in this week's release!)
There was a problem hiding this comment.
Sure, sounds good.
Sorry for the back and forth, I think this is now as minimal as it gets in terms of changes.
danhper
force-pushed
the
increase-native-buffer-size
branch
from
1a8a826 to
180359b
Compare
|
This seems like it is ok to me. Something is weird with the CI it seems |
…se-native-buffer-size
|
Looks like merging the most recent stuff on |
|
Thanks again for this, getting this into the release! |
3 participants
I was getting a
spawnSync /bin/sh ENOBUFSwhen trying to use the nativesolccommand to compile.It is because the stdin buffer size is 1MB by default and
execSyncwas trying to pass more data.I increased that to 10MB. It is far from perfect but at least it should do the job for most cases.
The other changes are because of eslint (git hook was failing). Let me know if you would prefer me to revert these.