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

Make types for OpenBSD/arm64 #993

Closed
wants to merge 2 commits into from
Closed

Conversation

vext01
Copy link

@vext01 vext01 commented Nov 9, 2020

As requested here.

These were generated on OpenBSD-current on a Raspberry Pi 4.

Can you tell me how to test the build and run the test suite? I don't know anything about golang!

Fixes #992

@Lomanic
Copy link
Collaborator

Lomanic commented Nov 9, 2020

Thanks a lot @vext01! I should buy a Raspberry 4 one day :)

You can test your changes with go build github.com/shirou/gopsutil/... (checking it builds and there is no missing types) and go test github.com/shirou/gopsutil/... (running tests). You should also generate types for v3 modules, cd v3 && sh ../mktypes.sh should do it (the mktypes.shscript has to be fixed to handle the v3 modules, I'm working on that).

@github-actions github-actions bot added the v3 label Nov 10, 2020
@vext01
Copy link
Author

vext01 commented Nov 10, 2020

OK, I've added a commit for the v3 types, but it doesn't appear to build.

In fact, I'm unsure if I'm doing this right. The struggles of someone who's never used golang 🆘

$ go build github.com/shirou/gopsutil/...                                                            
internal/common/common_openbsd.go:11:2: cannot find package "golang.org/x/sys/unix" in any of:      
        /usr/local/go/src/golang.org/x/sys/unix (from $GOROOT)                                      
        /home/edd/go/src/golang.org/x/sys/unix (from $GOPATH)         

So I guess we need to use go get?

$ go get github.com/shirou/gopsutil/...                                                              
# golang.org/x/sys/unix                                                                                                                                                                                             
SIGILL: illegal instruction                                                                               
PC=0x1aa194 m=2 sigcode=1                            
instruction bytes: 0x1b 0x28 0x85 0x52 0x3f 0x0 0x1b 0x6b 0xec 0x1 0x0 0x54 0x0 0x3c 0x40 0xd3

goroutine 36 [running]:                              
cmd/internal/obj/arm64.(*ctxt7).opldr12(0x4001199d30, 0x40001844c0, 0x2941, 0x50)
        /usr/local/go/src/cmd/internal/obj/arm64/asm7.go:6302 +0x184 fp=0x4001199840 sp=0x40011997f0 pc=0x1aa194
cmd/internal/obj/arm64.(*ctxt7).asmout(0x4001199d30, 0x40001844c0, 0xb90ef8, 0x4001199c88, 0x6, 0x6)
        /usr/local/go/src/cmd/internal/obj/arm64/asm7.go:3352 +0xa8d8 fp=0x4001199bc0 sp=0x4001199840 pc=0x1a5ae8
... loads more stacktrace

But it seems to be enough to run the build now:

$ go build github.com/shirou/gopsutil/...     
# github.com/shirou/gopsutil/disk
disk/disk_openbsd.go:90:36: cannot use d.Name[:] (type []uint8) as type []int8 in argument to common.IntToString
# github.com/shirou/gopsutil/v3/disk
v3/disk/disk_openbsd.go:90:36: cannot use d.Name[:] (type []uint8) as type []int8 in argument to common.IntToString
# github.com/shirou/gopsutil/v3/process
v3/process/process_openbsd.go:49:35: cannot use k.Comm[:] (type []uint8) as type []int8 in argument to common.IntToString
v3/process/process_openbsd.go:244:19: undefined: mem.GetpageSizeWithContext
# github.com/shirou/gopsutil/process
process/process_openbsd.go:49:35: cannot use k.Comm[:] (type []uint8) as type []int8 in argument to common.IntToString

@Lomanic
Copy link
Collaborator

Lomanic commented Nov 12, 2020

There's a type mismatch in the code generated by mktypes.sh, it doesn't build currently. I don't understand why yet, it looks like it's the char conversion that changed from int8 to uint8 (compare with the amd64 files). I should download a newer openbsd version than 6.5 to check that.

@vext01
Copy link
Author

vext01 commented Nov 12, 2020

It rings a bell that the signedness of a char varies between platforms.

@vext01
Copy link
Author

vext01 commented Nov 12, 2020

Yes, here:

The C and C++ standards allows the character type char to be signed or unsigned, depending on the platform and compiler. Most systems, including x86 GNU/Linux and Microsoft Windows, use signed char, but those based on PowerPC and ARM processors typically use unsigned char.

I don't know if the same semantics are expected of go?

@Lomanic
Copy link
Collaborator

Lomanic commented Nov 15, 2020

We generate some types from their C definition, so we inherit hat indeed, thanks for the pointer. We could just blindly cast the slices to uint8 like in the following diff

diff --git a/disk/disk_openbsd.go b/disk/disk_openbsd.go
index e675580..f71862f 100644
--- a/disk/disk_openbsd.go
+++ b/disk/disk_openbsd.go
@@ -87,7 +87,11 @@ func IOCountersWithContext(ctx context.Context, names ...string) (map[string]IOC
 		if err != nil {
 			continue
 		}
-		name := common.IntToString(d.Name[:])
+		uint8Name := make([]uint8, len(d.Name))
+		for i, v := range d.Name {
+			uint8Name[i] = uint8(v)
+		}
+		name := common.UintToString(uint8Name)
 
 		if len(names) > 0 && !common.StringsHas(names, name) {
 			continue
diff --git a/process/process_openbsd.go b/process/process_openbsd.go
index 0404b4b..c439d92 100644
--- a/process/process_openbsd.go
+++ b/process/process_openbsd.go
@@ -46,7 +46,11 @@ func (p *Process) NameWithContext(ctx context.Context) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	name := common.IntToString(k.Comm[:])
+	uint8Name := make([]uint8, len(k.Comm))
+	for i, v := range d.Name {
+		uint8Name[i] = uint8(v)
+	}
+	name := common.UintToString(uint8Name)
 
 	if len(name) >= 15 {
 		cmdlineSlice, err := p.CmdlineSliceWithContext(ctx)
diff --git a/v3/disk/disk_openbsd.go b/v3/disk/disk_openbsd.go
index 24324a4..7530901 100644
--- a/v3/disk/disk_openbsd.go
+++ b/v3/disk/disk_openbsd.go
@@ -87,7 +87,11 @@ func IOCountersWithContext(ctx context.Context, names ...string) (map[string]IOC
 		if err != nil {
 			continue
 		}
-		name := common.IntToString(d.Name[:])
+		uint8Name := make([]uint8, len(d.Name))
+		for i, v := range d.Name {
+			uint8Name[i] = uint8(v)
+		}
+		name := common.UintToString(uint8Name)
 
 		if len(names) > 0 && !common.StringsHas(names, name) {
 			continue
diff --git a/v3/process/process_openbsd.go b/v3/process/process_openbsd.go
index 7325f09..1b29204 100644
--- a/v3/process/process_openbsd.go
+++ b/v3/process/process_openbsd.go
@@ -46,7 +46,11 @@ func (p *Process) NameWithContext(ctx context.Context) (string, error) {
 	if err != nil {
 		return "", err
 	}
-	name := common.IntToString(k.Comm[:])
+	uint8Name := make([]uint8, len(k.Comm))
+	for i, v := range d.Name {
+		uint8Name[i] = uint8(v)
+	}
+	name := common.UintToString(uint8Name)
 
 	if len(name) >= 15 {
 		cmdlineSlice, err := p.CmdlineSliceWithContext(ctx)

At least it compiles from what I see now. Would test with these changes applied with the same commands as in #993 (comment) on your arm64 host on openbsd?

@Lomanic
Copy link
Collaborator

Lomanic commented Nov 15, 2020

I guess you also need the following diff for the tests to run but it's unrelated to your PR itself

diff --git a/v3/process/process_openbsd.go b/v3/process/process_openbsd.go
index 6e45d84..7325f09 100644
--- a/v3/process/process_openbsd.go
+++ b/v3/process/process_openbsd.go
@@ -241,7 +241,7 @@ func (p *Process) MemoryInfoWithContext(ctx context.Context) (*MemoryInfoStat, e
 	if err != nil {
 		return nil, err
 	}
-	pageSize, err := mem.GetpageSizeWithContext(ctx)
+	pageSize, err := mem.GetPageSizeWithContext(ctx)
 	if err != nil {
 		return nil, err
 	}

@vext01
Copy link
Author

vext01 commented Nov 18, 2020

Hi @Lomanic

It seems the patch doesn't apply:

$ patch -CEsp1 < patch1.diff  
1 out of 1 hunks failed
1 out of 1 hunks failed
1 out of 1 hunks failed
1 out of 1 hunks failed

@Lomanic Lomanic changed the title Make types for OpenBSD/arm64. Make types for OpenBSD/arm64 Nov 19, 2020
@Lomanic
Copy link
Collaborator

Lomanic commented Dec 2, 2020

This patch applies with git apply (git apply <patch1.diff). Otherwise, it's only a matter of search and replace.

The second patch is not needed anymore if you rebase from master (git pull --rebase origin master with origin as upstream remote).

@Lomanic
Copy link
Collaborator

Lomanic commented Dec 17, 2020

Hi @vext01, will you be able to test the changes in #993 (comment) (git apply or search & replace)?

@vext01
Copy link
Author

vext01 commented Dec 21, 2020

Before and after a rebase, I get:

$ git apply patch.diff
error: patch failed: disk/disk_openbsd.go:87
error: disk/disk_openbsd.go: patch does not apply
error: patch failed: process/process_openbsd.go:46
error: process/process_openbsd.go: patch does not apply
error: patch failed: v3/disk/disk_openbsd.go:87
error: v3/disk/disk_openbsd.go: patch does not apply
error: patch failed: v3/process/process_openbsd.go:46
error: v3/process/process_openbsd.go: patch does not apply

Can you make a branch containing the changes you'd like tested and I'll try that?

vext01 and others added 2 commits December 22, 2020 01:12
These were generated on OpenBSD-current on a Raspberry Pi 4.
@Lomanic
Copy link
Collaborator

Lomanic commented Dec 22, 2020

There you go https://github.com/Lomanic/gopsutil/tree/openbsd-arm64 (with your commits squashed and rebased from upstream master).

(It could have even been simpler to enable "repository maintainer permissions on existing pull requests", never used this function yet)

@vext01
Copy link
Author

vext01 commented Dec 23, 2020

Thanks.

I cloned your repo to ~/go/src/github.com/Lomanic/gopsutil, changed to your branch, and ran go test github.com/Lomanic/gopsutil/.... This is the output:

package github.com/Lomanic/gopsutil/cpu
        cpu/cpu.go:13:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/disk
        disk/disk.go:7:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/docker
        docker/docker.go:8:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/host
        host/host.go:10:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/load
        load/load.go:6:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/mem
        mem/mem.go:6:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/net
        net/net.go:8:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/process
        process/process.go:14:2: use of internal package github.com/shirou/gopsutil/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/cpu
        v3/cpu/cpu.go:13:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/disk
        v3/disk/disk.go:7:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/docker
        v3/docker/docker.go:8:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/host
        v3/host/host.go:10:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/load
        v3/load/load.go:6:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/mem
        v3/mem/mem.go:6:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/net
        v3/net/net.go:8:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed
package github.com/Lomanic/gopsutil/v3/process
        v3/process/process.go:14:2: use of internal package github.com/shirou/gopsutil/v3/internal/common not allowed

And it exits 1, so I guess something isn't right?

I also tried just go test ... in the source dir and saw similar errors.

@Lomanic
Copy link
Collaborator

Lomanic commented Dec 29, 2020

I cloned your repo to ~/go/src/github.com/Lomanic/gopsutil

You should have cloned it to ~/go/src/github.com/shirou/gopsutil, or if you already have gopsutil cloned there, add a remote with git remote add lomanic https://github.com/Lomanic/gopsutil and git fetch --all && git checkout lomanic/openbsd-arm64

@vext01
Copy link
Author

vext01 commented Dec 29, 2020

Oops. My apologies. I really know nothing about golang.

So, I think this is right:

$ pwd           
/home/edd/go/src/github.com/shirou/gopsutil
$ git remote -v 
origin  https://github.com/Lomanic/gopsutil (fetch)
origin  https://github.com/Lomanic/gopsutil (push)
$ PAGER= git branch
  master
* openbsd-arm64
$ git rev-parse HEAD
ec167b729e855a228821d553971684deb3d10459

Here's the output of the test (I had to go get github.com/stretchr/testify first because it was complaining it couldn't find it):

$ $ go test github.com/shirou/gopsutil/...
?   	github.com/shirou/gopsutil	[no test files]
--- FAIL: TestCpu_times (0.00s)
    cpu_test.go:62: {"cpu":"cpu-total","user":22.6,"system":0.0,"idle":0.0,"nice":0.0,"iowait":0.0,"irq":16.5,"softirq":0.0,"steal":0.0,"guest":0.0,"guestNice":0.0}
    cpu_test.go:68: 
        	Error Trace:	cpu_test.go:68
        	Error:      	Relative error is too high: 2 (expected)
        	            	        < 3.0008861320336724 (actual)
        	Test:       	TestCpu_times
--- FAIL: TestCpuInfo (0.00s)
    cpu_test.go:117: error operation not supported
    cpu_test.go:120: could not get CPU Info
FAIL
FAIL	github.com/shirou/gopsutil/cpu	0.306s
ok  	github.com/shirou/gopsutil/disk	(cached)
?   	github.com/shirou/gopsutil/docker	[no test files]
# github.com/shirou/gopsutil/process
process/process_openbsd.go:50:20: undefined: d
FAIL	github.com/shirou/gopsutil/host [build failed]
ok  	github.com/shirou/gopsutil/internal/common	(cached)
ok  	github.com/shirou/gopsutil/load	(cached)
ok  	github.com/shirou/gopsutil/mem	(cached)
ok  	github.com/shirou/gopsutil/net	(cached)
# github.com/shirou/gopsutil/process [github.com/shirou/gopsutil/process.test]
process/process_openbsd.go:50:20: undefined: d
FAIL	github.com/shirou/gopsutil/process [build failed]
--- FAIL: TestCpu_times (0.00s)
    cpu_test.go:62: {"cpu":"cpu-total","user":23.0,"system":0.0,"idle":0.0,"nice":0.0,"iowait":0.0,"irq":17.0,"softirq":0.0,"steal":0.0,"guest":0.0,"guestNice":0.0}
    cpu_test.go:68: 
        	Error Trace:	cpu_test.go:68
        	Error:      	Relative error is too high: 2 (expected)
        	            	        < 3.0008684324793737 (actual)
        	Test:       	TestCpu_times
--- FAIL: TestCpuInfo (0.00s)
    cpu_test.go:117: error operation not supported
    cpu_test.go:120: could not get CPU Info
FAIL
FAIL	github.com/shirou/gopsutil/v3/cpu	0.377s
ok  	github.com/shirou/gopsutil/v3/disk	(cached)
ok  	github.com/shirou/gopsutil/v3/docker	(cached)
# github.com/shirou/gopsutil/v3/process
v3/process/process_openbsd.go:50:20: undefined: d
FAIL	github.com/shirou/gopsutil/v3/host [build failed]
ok  	github.com/shirou/gopsutil/v3/internal/common	(cached)
ok  	github.com/shirou/gopsutil/v3/load	(cached)
ok  	github.com/shirou/gopsutil/v3/mem	(cached)
ok  	github.com/shirou/gopsutil/v3/net	(cached)
# github.com/shirou/gopsutil/v3/process [github.com/shirou/gopsutil/v3/process.test]
v3/process/process_openbsd.go:50:20: undefined: d
FAIL	github.com/shirou/gopsutil/v3/process [build failed]
FAIL

Looks like we are getting somewhere.

@Lomanic
Copy link
Collaborator

Lomanic commented Jan 18, 2021

Well, this looks quite promising in fact, thanks for testing this. The failing tests are unrelated to the changes, these tests should be improved as they are also often failing on Github Actions. Would you force-push ec167b7 to your branch @vext01 then?

@vext01
Copy link
Author

vext01 commented Jan 25, 2021

Done! Sorry for the delay!

@vext01
Copy link
Author

vext01 commented Feb 23, 2021

Let's not forget this one.

Lomanic added a commit to Lomanic/gopsutil that referenced this pull request Feb 23, 2021
Lomanic added a commit to Lomanic/gopsutil that referenced this pull request Feb 23, 2021
@Lomanic
Copy link
Collaborator

Lomanic commented Feb 23, 2021

Hi, thanks for the reminder, I overlooked it in my last message in fact, as I didn't see the following error

process/process_openbsd.go:50:20: undefined: d
FAIL	github.com/shirou/gopsutil/host [build failed]

I did a silly copy-paste mistake it seems (from the disk package I can see now), I updated my branch at https://github.com/Lomanic/gopsutil/tree/openbsd-arm64 (I'm not sure I can push to your branch) and I could test it on my old OpenBSD 6.5 i386 VM this time, without issues ; the process package now builds and has its tests run successfully.

If you can confirm me that tests run successfully on your arm64 host also (minus the TestCpu_times failing test, it's unrelated), it looks like we will finally be able to merge this. Thanks for staying for so long over here, appreciated.

@tobias-hildebrandt
Copy link

Hello, I'm running OpenBSD -current on my Raspberry Pi 4 (2GB RAM version).
I hope I am able to help with my machine's test results.

I've run go test ./... on Lomanic's openbsd-arm64 branch and I've attached a log.
I've also attached the output of my go env.
The output of my uname -a is OpenBSD rpi4-openbsd.none 6.9 GENERIC.MP#1051 arm64.
I am also pretty clueless about Go, so let me know if there is anything else I should run.

Lomanic added a commit to Lomanic/gopsutil that referenced this pull request Mar 23, 2021
ivandeex added a commit to rclone/rclone that referenced this pull request Apr 5, 2021
Related to #5121

Note: OpenBSD is stub yet. This will be fixed after upstream PR gets resolved
shirou/gopsutil#993
@Lomanic
Copy link
Collaborator

Lomanic commented Apr 19, 2021

superseded by #1052

@Lomanic Lomanic closed this Apr 19, 2021
negative0 pushed a commit to negative0/rclone that referenced this pull request Aug 13, 2021
Related to rclone#5121

Note: OpenBSD is stub yet. This will be fixed after upstream PR gets resolved
shirou/gopsutil#993
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.

Build failure on OpenBSD/aarch64
3 participants