Allow to be built with CGO_ENABLE=0 #345

xchenan · 2017-05-02T18:17:03Z

Re-implemented the parts that included codes in C. Now the projects should be in pure golang.

The re-implementation of termios refers to go-termios: https://github.com/go-termios/termios.

Signed-off-by: xchenan <xchenan@gmail.com>

xiaq · 2017-05-03T18:25:51Z

Hi, thanks for the PR. I am pretty busy ATM and will try to get to you this weekend.

xiaq · 2017-05-14T21:55:19Z

First of all, thank you for doing this! We are using sqlite which also requires cgo, but it's good to get rid of cgo for the rest of the code. Read below for comments.

Please vendor the dependencies (github.com/go-termios/termios github.com/mattn/is-atty golang.org/x/sys/unix) under the vendor/ directory. Go doesn't have a standard dependency versioning tool yet, vendoring is the only way to get reproducible builds. Also, once you do this, Travis CI will be able to test your change.
Please make sure Elvish compile on both Linux and macOS. Once you have done the above, Travis will do this for you. It is worth pointing out that go-termios does not seem to have a macOS implementation. It is quite likely that the freebsd version will work for macOS unmodified, though. If not you will need to pull the constants off the header files on a macOS system.

…d issues on MacOS Signed-off-by: xchenan <xchenan@gmail.com>

xchenan · 2017-05-19T17:19:14Z

@xiaq Just a kind reminder that the PR works for both linux and macOS now.

xiaq

Sorry for the delay, and thanks again.

Overall the PR is in a good shape; see comments.

xiaq · 2017-05-19T23:12:15Z

sys/select_bsd.go

@@ -4,6 +4,13 @@ package sys

 import "syscall"

+const NFDBits = 32


This hardcoding seems unnecessary. Please use unsafe.Sizeof to find out the size. You wouldn't need to split select_bsd.go and select_linux.go either.

On MacOS, FdSet.Bits is []int32 whereas on linux FdSet.Bits is []int64. Therefore IMHO the return type of func index() shall be different based on the types of FdSet.Bits on different OS. Is there an alternative to solve this without splitting the code?

I overlooked the difficulty here. Let's leave it separated.

unsafe.Sizeof(u) returns 8 on both Linux and MacOS. But NFDBits needs to be 64 on Linux and 32 on MacOS. Since we have to separate the impl anyway I just left NFDBits hard-coded.

xiaq · 2017-05-19T23:17:53Z

sys/termios_bsd.go

+	TCIOFF = 3
+	TCION  = 4
+)
+


Only consonants in the const block below and setFlag are used.

Please stripe the file and (termios_linux.go) to include only these consonants (see setFlag for why it should also be removed). Since these codes are forked instead of vendored, developers of Elvish will be maintaining these code, and we should maintain as little code as possible. :)

xiaq · 2017-05-19T23:19:59Z

sys/termios_bsd.go

+//	}
+//}
+
+func setFlag(flag *uint64, mask uint64, v bool) {


This seems to unnecessarily rely on the actual type of term.LFlag.

If you inline the function manually, Go's type inference will do the correct thing.

The problem of inlining the function is that, we are not able to negate constants (unix.ICANON and unix.ECHO) in Go. A possible way of doing this is to valuate a variable with value of the constant, then use the variable instead. However the types of such variable needs to be defined differently on MacOS and Linux. I suppose a better way of doing this is to define setFlag() separately for different OS.

You are right. Let's leave this one separated too.

I just left them separated.

xiaq · 2017-05-19T23:23:25Z

sys/winsize.go


 // GetWinsize queries the size of the terminal referenced by the given file
 // descriptor.
+//
+type winSize struct {


Please move this struct above GetWinsize's docstring.

Also, this is one of the few places where we have to rely on the size of the fields. Please include a comment like this:

// winSize mirrors struct winsize in the C header. The following declaration matches struct winsize in the headers of Linux and FreeBSD.

(I've verified that.)

xiaq · 2017-05-19T23:25:06Z

sys/errors.go

@@ -0,0 +1,8 @@
+package sys


Please remove this file. ErrNotImplemented is not used, and ErrInvalidAction will not be used if you remove other unused code (see below).

xiaq · 2017-05-19T23:37:48Z

Also, please remove your vim swap file from the PR :)

xchenan

I have some concerns on your kind suggestions. I may need further clarification.

xchenan · 2017-05-22T16:48:15Z

sys/select_bsd.go

@@ -4,6 +4,13 @@ package sys

 import "syscall"

+const NFDBits = 32


On MacOS, FdSet.Bits is []int32 whereas on linux FdSet.Bits is []int64. Therefore IMHO the return type of func index() shall be different based on the types of FdSet.Bits on different OS. Is there an alternative to solve this without splitting the code?

xchenan · 2017-05-22T18:53:17Z

sys/termios_bsd.go

+//	}
+//}
+
+func setFlag(flag *uint64, mask uint64, v bool) {


The problem of inlining the function is that, we are not able to negate constants (unix.ICANON and unix.ECHO) in Go. A possible way of doing this is to valuate a variable with value of the constant, then use the variable instead. However the types of such variable needs to be defined differently on MacOS and Linux. I suppose a better way of doing this is to define setFlag() separately for different OS.

xiaq · 2017-05-22T22:52:40Z

I left comments, in case GitHub didn't send an email. You are right in the two comments, I overlooked the problems.

…in termios_linux.go and termios_bsd.go; Signed-off-by: xchenan <xchenan@gmail.com>

xchenan · 2017-05-23T00:25:11Z

@xiaq I fixed the code based on your comments. Hopefully good to go now.

xiaq · 2017-05-23T00:29:28Z

The story of NFDBits is more complex than I thought. Linux doesn't always have 64-bit Bits arrays in FdSet; nor does BSDs have 32-bit ones. In your select_{linux,bsd}.go, the second return value of index, bit int64 is problematic, because you will be using this value with an element of Bits in select.go; if the types don't match, the code won't compile. This is currently the case on 32bit Linux, where the code assumes int64 whereas the definition is really int32.

I think the solution would be that for this function, you need to have two files, select_32.go and select_64.go to distinguish the platforms where Bits is an array of int32 and where it is an array of int64. You can get this information by inspecting its definition in the source code of the syscall package: https://github.com/golang/go/tree/964639cc338db650ccadeafb7424bc8ebb2c0f6c/src/syscall

For you convenience, among the arch/kernel combinations that I care about now, the following have 32-bit Bits:

// +build darwin 386,freebsd arm,freebsd 386,linux arm,linux netbsd openbsd

The following have 64-bit Bits:

// +build amd64,dragonfly amd64,freebsd amd64,linux arm64,linux

In those two files you can hardcode NFDBits. Please also leave comments in those two files explaining the reason we are doing this.

xiaq · 2017-05-23T00:34:33Z

sys/winsize.go

 type winSize struct {
 	row    uint16
 	col    uint16
 	Xpixel uint16
 	Ypixel uint16
 }

+// GetWinsize queries the size of the terminal referenced by
+// the given file descriptor.
+


Remove this blank line, that is how Go docstrings should be.

xiaq · 2017-05-23T01:13:31Z

sys/select_32.go

@@ -1,4 +1,4 @@
-// +build darwin dragonfly freebsd netbsd openbsd
+// +build darwin 386,freebsd arm,freebsd 386,linux arm,linux netbsd openbsd


You need both select_{bsd,linux}.go and select_{32,64}.go. The first pair is due to the fact that Select has different signatures on BSD and Linux.

xiaq · 2017-05-23T01:13:53Z

Remember to remove your vim swap file.

different architectures. Signed-off-by: xchenan <xchenan@gmail.com>

xchenan · 2017-05-23T17:44:53Z

Sorry about the back and forth. Please let me know if there's more I need to modify.

xiaq · 2017-05-23T19:23:03Z

Aargh...

The implementation of setFlag, for manipulating Termios also needs to split itself between 32-bit and 64-bit types LFlag field.

Apparently, for what we care about, only amd64,darwin has LFlag uint64; every other platform has uint32. You can make a termios_lflag_32.go and termios_lfag_64.go for that.

I'll merge your PR though, the process is taking longer than I like and I will fix that in a later commit.

xiaq · 2017-05-23T19:26:59Z

And thanks again for doing this!

xchenan · 2017-05-23T20:28:59Z

Appreciations. I am truly glad for all the kind comments and, of course, the approval.

Re-implemented in pure golang.

a4dddc9

Signed-off-by: xchenan <xchenan@gmail.com>

xchenan force-pushed the pure-go-step-by-step branch from 1665a79 to a4dddc9 Compare May 2, 2017 18:21

xchenan force-pushed the pure-go-step-by-step branch 5 times, most recently from 9a5d5a4 to 7d432ae Compare May 15, 2017 22:52

Vendored github.com/mattn/go-isatty/ and golang.org/x/sys/unix/; Fixe…

18041b0

…d issues on MacOS Signed-off-by: xchenan <xchenan@gmail.com>

xchenan force-pushed the pure-go-step-by-step branch from 7d432ae to 18041b0 Compare May 15, 2017 23:17

xiaq requested changes May 19, 2017

View reviewed changes

xchenan commented May 22, 2017

View reviewed changes

xchenan force-pushed the pure-go-step-by-step branch from 6835461 to 83b84c1 Compare May 22, 2017 23:55

Added comments in winsize.go; Removed unused functions and constants …

519b7e5

…in termios_linux.go and termios_bsd.go; Signed-off-by: xchenan <xchenan@gmail.com>

xchenan force-pushed the pure-go-step-by-step branch from 83b84c1 to 519b7e5 Compare May 23, 2017 00:13

xiaq reviewed May 23, 2017

View reviewed changes

Handled distinctive NFDBits and func index's return values according to

0b87620

different architectures. Signed-off-by: xchenan <xchenan@gmail.com>

xchenan force-pushed the pure-go-step-by-step branch from 7f685c0 to 0b87620 Compare May 23, 2017 16:39

xiaq merged commit cf12ae3 into elves:master May 23, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Allow to be built with CGO_ENABLE=0 #345

Allow to be built with CGO_ENABLE=0 #345

xchenan commented May 2, 2017

xiaq commented May 3, 2017

xiaq commented May 14, 2017

xchenan commented May 19, 2017

xiaq left a comment

xiaq May 19, 2017

xchenan May 22, 2017

xiaq May 22, 2017

xchenan May 23, 2017

xiaq May 19, 2017

xchenan May 23, 2017

xiaq May 19, 2017

xchenan May 22, 2017

xiaq May 22, 2017

xchenan May 23, 2017

xiaq May 19, 2017

xchenan May 23, 2017

xiaq May 19, 2017

xchenan May 23, 2017

xiaq commented May 19, 2017

xchenan left a comment

xchenan May 22, 2017

xchenan May 22, 2017

xiaq commented May 22, 2017

xchenan commented May 23, 2017

xiaq commented May 23, 2017

xiaq May 23, 2017

xiaq May 23, 2017

xiaq commented May 23, 2017

xchenan commented May 23, 2017

xiaq commented May 23, 2017

xiaq commented May 23, 2017

xchenan commented May 23, 2017

		@@ -4,6 +4,13 @@ package sys

		import "syscall"

		const NFDBits = 32

		@@ -1,4 +1,4 @@
		// +build darwin dragonfly freebsd netbsd openbsd
		// +build darwin 386,freebsd arm,freebsd 386,linux arm,linux netbsd openbsd

Allow to be built with CGO_ENABLE=0 #345

Allow to be built with CGO_ENABLE=0 #345

Conversation

xchenan commented May 2, 2017

xiaq commented May 3, 2017

xiaq commented May 14, 2017

xchenan commented May 19, 2017

xiaq left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaq commented May 19, 2017

xchenan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaq commented May 22, 2017

xchenan commented May 23, 2017

xiaq commented May 23, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

xiaq commented May 23, 2017

xchenan commented May 23, 2017

xiaq commented May 23, 2017

xiaq commented May 23, 2017

xchenan commented May 23, 2017