search

hyperledger-solang / solang

Solidity Compiler for Solana and Polkadot
https://solang.readthedocs.io/
Apache License 2.0
1.27k stars 215 forks source link

feat(parser): exclude build.rs, include auto-generated parser on crates.io #1619

Closed DaniPopes closed 8 months ago

DaniPopes commented 9 months ago

Helps with compile times by removing dependency on lalrpop and build script when using solang-parser from crates.io. Downside is this is 2MB in tree, but this shouldn't be that big of a deal.

seanyoung commented 9 months ago

It's a nice idea to improve build times. However, if the grammar changes or there a (minor) lalrpop release, it has to been re-generated. Note that solang had this file checked in when we started a few years ago, but it's pretty ugly when you're reviewing PRs.

Also from an audit perspective, this is not good.

I guess the question is, is generating the parser really that time-consuming?

seanyoung commented 9 months ago

See https://github.com/hyperledger/solang/commit/d3f64587fb88717e6d670ea7fc4397269e8c2797 and https://github.com/hyperledger/solang/commit/da3aa8773b9e19c86467710e759f6a644d3a78fe

That commit mentions another problem: cargo publish does not like it

DaniPopes commented 9 months ago

if the grammar changes

This is not a problem because it can only change locally and the build script will re-run.

or there a (minor) lalrpop release

Good point, we would have to lock it to a specific version. I can only see this being a problem if other dependencies in the same project use another version which would duplicate it.

but it's pretty ugly when you're reviewing PRs

I can't really see how, it's hidden in reviews because it's too big by default. I guess the diff numbers are annoying?

I guess the question is, is generating the parser really that time-consuming?

On my machine it's about 5 seconds to compile lalrpop and another 5 to compile and run the build script, although admittedly it can run in parallel and might not always be a bottleneck, but it is decently time-consuming nonetheless.

I wasn't aware that you were doing this initially, but I believe cargo publish / docs.rs shouldn't be problems anymore.

In any case if you think this is not worth it / not doable, feel free to close this PR.

xermicus commented 8 months ago

I wasn't aware that you were doing this initially, but I believe cargo publish / docs.rs shouldn't be problems anymore.

Are we sure about that? It'd be a hard blocker.

Also from an audit perspective, this is not good.

Very good point. That creates a perfect opportunity to smuggle in anything in by anyone who touches it. It's too hard to audit this chunky file every time it changes. Or is it reproducible (I think it should be right?) so that we can at least confirm its checksum with a version we generated in our CI or something like that.

I hate long compile times, but in this case I'm leaning more towards not doing this, not sure if its worth the trouble.