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

cmd/geth, cmd/utils: remove legacy rpc flags #23358

Merged
merged 25 commits into from Sep 7, 2021

Conversation

Zachinquarantine
Copy link
Contributor

Closes #23267

This PR aims to remove the legacy RPC flags that say "deprecated, will be removed in June 2021".

@Zachinquarantine
Copy link
Contributor Author

just curious, can I remove the rpcport flag in cmd/clef/main.go or is that seperate from the deprecated --rpcport flag?

@Zachinquarantine Zachinquarantine marked this pull request as ready for review August 10, 2021 01:43
@Zachinquarantine
Copy link
Contributor Author

am I missing something I need to remove? I've looked, and I honestly can't tell.

@karalabe
Copy link
Member

The code doesn't compile.

@Zachinquarantine Zachinquarantine changed the title [WIP] all: remove legacy rpc flags all: remove legacy rpc flags Aug 10, 2021
@Zachinquarantine
Copy link
Contributor Author

Zachinquarantine commented Aug 10, 2021

I'm struggling to figure out what went wrong here:
cmd/utils/flags.go:1787:2: expected declaration, found 'switch' (typecheck)

switch {

^

@Zachinquarantine
Copy link
Contributor Author

travis ci seems to be stuck...

@Zachinquarantine
Copy link
Contributor Author

just curious, can I remove the rpcport flag in cmd/clef/main.go or is that seperate from the deprecated --rpcport flag?

cmd/geth/usage.go Show resolved Hide resolved
cmd/geth/usage.go Outdated Show resolved Hide resolved
cmd/geth/usage.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
cmd/utils/flags.go Outdated Show resolved Hide resolved
reverts the spacing to how it was before I resolved the merge conflict
@Zachinquarantine
Copy link
Contributor Author

can I get a review and/or merge for this, please?

@MariusVanDerWijden
Copy link
Member

@Zachinquarantine This PR does not compile. It doesn't even lint: https://app.travis-ci.com/github/ethereum/go-ethereum/jobs/533060458
You should set up your go environment in a way that automatically lints and run go test ./... before you submit your PRs.

cmd/geth/usage.go Outdated Show resolved Hide resolved
cmd/geth/usage.go Outdated Show resolved Hide resolved
cmd/geth/usage.go Outdated Show resolved Hide resolved
fix for cmd/geth/usage.go:243:17: expected operand, found ':=' (typecheck) in travis
@holiman
Copy link
Contributor

holiman commented Aug 25, 2021

cmd/utils/flags.go:923:2: expected declaration, found 'if' (typecheck)
	if ctx.GlobalBool(HTTPEnabledFlag.Name) && cfg.HTTPHost == "" {
	^
cmd/utils/flags.go:1082:2: expected declaration, found 'if' (typecheck)
	if ctx.GlobalIsSet(MinerEtherbaseFlag.Name) {
	^
cmd/utils/flags.go:1401:3: expected declaration, found 'if' (typecheck)
		if err = hash.UnmarshalText([]byte(parts[1])); err != nil {
		^
cmd/utils/flags.go:1423:36: expected 'IDENT', found ')' (typecheck)
			switch option := args[i+1].(type) {
			                                ^
cmd/utils/flags.go:1424:15: expected ';', found ':' (typecheck)
			case string:

You should be able to detect compilation issues via make geth or make all. And as others have pointed out running the tests locally is often a good idea.

@Zachinquarantine
Copy link
Contributor Author

cmd/utils/flags.go:923:2: expected declaration, found 'if' (typecheck)
	if ctx.GlobalBool(HTTPEnabledFlag.Name) && cfg.HTTPHost == "" {
	^
cmd/utils/flags.go:1082:2: expected declaration, found 'if' (typecheck)
	if ctx.GlobalIsSet(MinerEtherbaseFlag.Name) {
	^
cmd/utils/flags.go:1401:3: expected declaration, found 'if' (typecheck)
		if err = hash.UnmarshalText([]byte(parts[1])); err != nil {
		^
cmd/utils/flags.go:1423:36: expected 'IDENT', found ')' (typecheck)
			switch option := args[i+1].(type) {
			                                ^
cmd/utils/flags.go:1424:15: expected ';', found ':' (typecheck)
			case string:

You should be able to detect compilation issues via make geth or make all. And as others have pointed out running the tests locally is often a good idea.

I've ran the tests locally, and this is the only error the compiler is giving me:

cmd/utils/flags.go:923:2: expected declaration, found 'if' (typecheck)
if ctx.GlobalBool(HTTPEnabledFlag.Name) && cfg.HTTPHost == "" {

cmd/utils/flags.go Outdated Show resolved Hide resolved
Zachinquarantine and others added 2 commits August 25, 2021 10:19
Co-authored-by: Martin Holst Swende <martin@swende.se>
 fixes these errors:
cmd/utils/flags_legacy.go:21:2: "strings" imported but not used (typecheck)

	"strings"

	^

cmd/utils/flags_legacy.go:24:2: "github.com/ethereum/go-ethereum/node" imported but not used (typecheck)

	"github.com/ethereum/go-ethereum/node"

	^
@MariusVanDerWijden
Copy link
Member

The linter is still red: https://app.travis-ci.com/github/ethereum/go-ethereum/jobs/533787275
You need to goimport -ed your files. What kind of IDE are you using? Goland and VSCode should do that automatically for you

@Zachinquarantine
Copy link
Contributor Author

The linter is still red: app.travis-ci.com/github/ethereum/go-ethereum/jobs/533787275
You need to goimport -ed your files. What kind of IDE are you using? Goland and VSCode should do that automatically for you

I'm using VSCode, but for some reason it hasn't fixed the goimports error when I save the file.

@Zachinquarantine Zachinquarantine changed the title all: remove legacy rpc flags cmd/geth, cmd/utils: remove legacy rpc flags Aug 26, 2021
@Zachinquarantine
Copy link
Contributor Author

Zachinquarantine commented Aug 26, 2021

The linter is still red: app.travis-ci.com/github/ethereum/go-ethereum/jobs/533787275
You need to goimport -ed your files. What kind of IDE are you using? Goland and VSCode should do that automatically for you

I'm using VSCode, but for some reason it hasn't fixed the goimports error when I save the file.

Turns out it was because I hadn't configured it properly, it now passes 🎉 @MariusVanDerWijden

Copy link
Member

@MariusVanDerWijden MariusVanDerWijden left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM
(This will probably break a lot of users though, as there are a lot of tutorials out there with these legacy flags)

@holiman holiman merged commit 9a0df80 into ethereum:master Sep 7, 2021
@holiman holiman added this to the 1.10.9 milestone Sep 7, 2021
@Zachinquarantine Zachinquarantine deleted the remove-legacy-rpc branch September 7, 2021 12:19
@collinpowell
Copy link

Please how do i get my rpc port default is not connecting to metamask

yongjun925 pushed a commit to DODOEX/go-ethereum that referenced this pull request Dec 3, 2022
* remove rpc flags

* remove legacy rpc flags

* remove legacy rpc flags

* remove legacy rpc commands

* (hopefully) fix most of the build errors

* fix build errors

https://app.travis-ci.com/github/ethereum/go-ethereum/jobs/530318686

* cmd/utils: fix syntax error

* empty commit to unbreak travis ci

* fix syntax error

* syntax fixes

* syntax fixes

* fix

fixes "cmd/geth/usage.go:234:7: expected '(', found init (typecheck)"

* fix

* various fixes in usage.go

* various fixes in flags.go

* adds extra space

reverts the spacing to how it was before I resolved the merge conflict

* more fixes in usage.go

* fix

fix for cmd/geth/usage.go:243:17: expected operand, found ':=' (typecheck) in travis

* Update cmd/utils/flags.go

Co-authored-by: Martin Holst Swende <martin@swende.se>

* fix error

 fixes these errors:
cmd/utils/flags_legacy.go:21:2: "strings" imported but not used (typecheck)

	"strings"

	^

cmd/utils/flags_legacy.go:24:2: "github.com/ethereum/go-ethereum/node" imported but not used (typecheck)

	"github.com/ethereum/go-ethereum/node"

	^

* goimports

Co-authored-by: Martin Holst Swende <martin@swende.se>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove rpccorsdomain and rpcapi
7 participants