Skip to content

build: enable unconvert linter#15456

Merged
karalabe merged 2 commits into
ethereum:masterfrom
ferhatelmas:enable-unconvert
Nov 10, 2017
Merged

build: enable unconvert linter#15456
karalabe merged 2 commits into
ethereum:masterfrom
ferhatelmas:enable-unconvert

Conversation

@ferhatelmas
Copy link
Copy Markdown
Contributor

 - fixes ethereum#15453
 - update code base for failing cases
@karalabe
Copy link
Copy Markdown
Member

Hah, there's an interesting corner case (https://ci.appveyor.com/project/ethereum/go-ethereum/build/master.4274/job/sl0p87958g9ajh6q#L199). On Linux, syscall.Stdin is an int, but on Windows, the same variable has a type of syscall.Handle. As such, the conversion is needed on Windows, but not on Linux. Need to think about how to get around this..

Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

Figured out how to fix the Stdin issue, please take a look at the review comments.

Comment thread cmd/puppeth/ssh.go Outdated
auths = append(auths, ssh.PasswordCallback(func() (string, error) {
fmt.Printf("What's the login password for %s at %s? (won't be echoed)\n> ", login, server)
blob, err := terminal.ReadPassword(int(syscall.Stdin))
blob, err := terminal.ReadPassword(syscall.Stdin)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

syscall.Stdin is problematic because it's a different type on Linux and Windows. Lets use int(os.Stdin.Fd()) here which will work on both platforms and won't cause unconvert to whine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Thanks, updated so.

Comment thread cmd/puppeth/wizard.go Outdated
func (w *wizard) readPassword() string {
fmt.Printf("> ")
text, err := terminal.ReadPassword(int(syscall.Stdin))
text, err := terminal.ReadPassword(syscall.Stdin)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

syscall.Stdin is problematic because it's a different type on Linux and Windows. Lets use int(os.Stdin.Fd()) here which will work on both platforms and won't cause unconvert to whine.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Same as other.

@karalabe karalabe added this to the 1.7.3 milestone Nov 10, 2017
@ferhatelmas
Copy link
Copy Markdown
Contributor Author

ferhatelmas commented Nov 10, 2017

@karalabe I also tried --all as you wrote false positives with stdin (#26) but couldn't get it worked.

Should we wait for upstream fix or continue with your suggested fix ?

@karalabe
Copy link
Copy Markdown
Member

I'm not sure how much work or time it would be to get unconvert patched up.

IMHO the workaround is fine and int's not really ugly or anything, so it shouldn't matter.

@ferhatelmas
Copy link
Copy Markdown
Contributor Author

I feel same. That's why I updated.

I also subscribed to unconvert issue. I can update this to call with --all flag when it's done.

Copy link
Copy Markdown
Member

@karalabe karalabe left a comment

Choose a reason for hiding this comment

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

LGTM

@karalabe karalabe merged commit 86f6568 into ethereum:master Nov 10, 2017
@ferhatelmas ferhatelmas deleted the enable-unconvert branch November 10, 2017 17:15
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.

Enable unconvert linter, fix detected issues

2 participants