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

allow passing custom compiler args #24925

Closed
wants to merge 1 commit into from

Conversation

krasi-georgiev
Copy link

Signed-off-by: Krasi Georgiev 8903888+krasi-georgiev@users.noreply.github.com

@krasi-georgiev krasi-georgiev marked this pull request as draft May 21, 2022 19:19
@krasi-georgiev
Copy link
Author

krasi-georgiev commented May 21, 2022

still working out the details, just checking if you are willing to merge such a PR?

Signed-off-by: Krasi Georgiev <8903888+krasi-georgiev@users.noreply.github.com>
@holiman
Copy link
Contributor

holiman commented May 23, 2022

just checking if you are willing to merge such a PR?

Not so sure about that -- but I guess that depends on the usecase. I'm sceptical as to why we need compiler integration in the first place, rather than have the user compile whatever he desires with whatever compiler he desires, and then just provide geth with the artefacts.

Then we could remove a lot of code, and we would not have to maintain and update the compiler integration in the coming years.

But maybe you have some good arguments and insights into why that would be a bad idea, and wouldn't work in some cases? I'm curious, since I've never used this feature myself, I really do not have any intuition about how people use it and why.

@krasi-georgiev
Copy link
Author

krasi-georgiev commented May 23, 2022

provide geth with the artefacts.
did you mean to provide abigen with the artifacts for generating the bindings?

Removing the compilation part means removing the ./common/compiler package, which is like 200-300lines of code. If you want to remove this extra maintenance, you can refactor abigen to require the artifacts folder.

I have built my own wrapper which adds downloading the contracts from etherscan directly, downloading the required compiler and generating the bindings. It uses the compiler package from geth, but I can duplicate that code if you are thinking of removing it.

https://github.com/cryptoriums/contraget/blob/main/pkg/contraget/contraget.go

@holiman
Copy link
Contributor

holiman commented May 23, 2022

provide geth with the artefacts.

did you mean to provide abigen with the artifacts for generating the bindings?

Yes, I meant that geth would only look at the compiler outputs, the abi definitions, but not try to orchestrate the compilers.

@krasi-georgiev
Copy link
Author

that sounds reasonable, I can refactor the PR to do that and as an addition can move the compile package in a repo I maintain in case people prefer to still use this wrapper

https://github.com/cryptoriums/packages

@karalabe
Copy link
Member

The compiler calls were removed on master, so this PR is not not relevant any more. Sorry for the wasted effort, though your PR did prompt cleaning up our code a bit, so I guess that's a plus :)

@karalabe karalabe closed this May 24, 2022
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.

3 participants