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

Jump to bottom

x/*: review tests skipped on darwin/arm64 after Go 1.17 release #45696

Closed
tklauser opened this issue Apr 22, 2021 · 15 comments
Closed

x/*: review tests skipped on darwin/arm64 after Go 1.17 release #45696

tklauser opened this issue Apr 22, 2021 · 15 comments
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Milestone
Go1.18

Comments

@tklauser
Copy link
Member

tklauser commented Apr 22, 2021 •
edited

Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 anddarwin/arm64 was repurposed for the macOS ARM64 port (see https://golang.org/doc/go1.16#darwin). Thegolang.org/x/* repositories were updated to use the new ios build tag to skip certain tests on IOS, but they also needed to retain the old runtime.GOOS == "darwin" && (runtime.GOARCH == "arm64" || runtime.GOARCH == "arm") checks due to Go 1.15 compatibilty. In some cases this leads to tests being unnecessarily skipped with Go 1.16 on darwin/arm64, see e.g. https://golang.org/cl/312250.

Once Go 1.17 is released and thus Go 1.16 becomes the oldest supported release per https://golang.org/doc/devel/release.html#policy, we might be able to remove some of the old GOOS == "darwin" && (GOARCH == "arm64" || GOARCH === "arm") checks and thus increase the test coverage for the macOS ARM64 port in the golang.org/x/* repos.

Some examples:

https://go.googlesource.com/sys/+/33663a62ff0820461192226283cafe57a83ac6b7/unix/syscall_unix_test.go#169
https://go.googlesource.com/sys/+/33663a62ff0820461192226283cafe57a83ac6b7/unix/syscall_unix_test.go#491
https://go.googlesource.com/net/+/4e50805a0758a198d5c9ea96b2e8dacd7338f2ee/nettest/nettest.go#102
https://go.googlesource.com/term/+/f5beecf764ed751c785fa21705385df593b1852d/terminal_test.go#410
@tklauser tklauser added OS-Darwin NeedsFix The path to resolution is known, but the work has not been done. labels Apr 22, 2021
@tklauser tklauser added this to the Go1.18 milestone Apr 22, 2021
@anton-kuklin
Copy link
Contributor

Hi @tklauser, I'll work on it.

@tklauser
Copy link
Member Author

@justplesh Thanks. Please note that we can only submit these changes once Go 1.17 is released, otherwise we'd break tests on Go 1.15 which is still supported until then. This issue for now is many here so we don't forget to review these tests once Go 1.17 is out.

@anton-kuklin
Copy link
Contributor

@tklauser Okay, sure, got you. How do you think, does it make sense to create the PR with changes now and merge it as soon as Go 1.17 will be released or a lot of changes will be made and it will make more sense to wait until the 1.17 release?

@tklauser
Copy link
Member Author

I think it's probably easier to send these changes once 1.17 is released. That way we can make sure all cases are covered and it might safe some trouble with rebases and/or merge conflicts.

@anton-kuklin
Copy link
Contributor

Okay, I'll get back as soon, as 1.17 will be released :)

@anton-kuklin
Copy link
Contributor

@tklauser so I guess I can start working on it?

@tklauser
Copy link
Member Author

@justplesh Yes, feel free to send CLs addressing this.

gopherbot pushed a commit to golang/term that referenced this issue Sep 27, 2021
@tklauser
term: enable TestMakeRawState on darwin/arm64
f766a8b 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

Now that Go 1.16 is the oldest supported release, the ios tag can be
used exclusively to detect iOS and TestMakeRawState which ought to run
on darwin/arm64 can be enabled on that platform.

For golang/go#45696

Change-Id: Ic51903ea94def1f1144ca74db37533b5c4de8522
Reviewed-on: https://go-review.googlesource.com/c/term/+/352589
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/352589 mentions this issue: term: enable TestMakeRawState on darwin/arm64

@gopherbot
Copy link

Change https://golang.org/cl/353509 mentions this issue: unix: enable Sysv shared memory support on darwin/arm64

@gopherbot
Copy link

Change https://golang.org/cl/353529 mentions this issue: unix: enable TestPassFD and TestPoll on darwin/arm64

@gopherbot
Copy link

Change https://golang.org/cl/353530 mentions this issue: cpu: enable TestARM64minimalFeatures on darwin/arm64

gopherbot pushed a commit to golang/sys that referenced this issue Oct 2, 2021
@tklauser
unix: enable TestPassFD and TestPoll on darwin/arm64
b1447f3 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

Now that Go 1.16 is the oldest supported release, the ios tag can be
used exclusively to detect iOS. Thus, TestPassFD and TestPoll which
ought to run on darwin/arm64 can now be enabled on that platform.

For golang/go#45696

Change-Id: I0b542af7a1b5ab194bf249d499c49e45bb55a2e1
Reviewed-on: https://go-review.googlesource.com/c/sys/+/353529
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Oct 2, 2021
@tklauser
cpu: enable TestARM64minimalFeatures on darwin/arm64
808efd9 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

Now that Go 1.16 is the oldest supported release, the ios tag can be
used exclusively to detect iOS. Thus, TestARM64minimalFeatures which
ought to run on darwin/arm64 can now be enabled on that platform.

For golang/go#45696

Change-Id: Ic510fbf27dc813832507446201501df58c9f6f31
Reviewed-on: https://go-review.googlesource.com/c/sys/+/353530
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
gopherbot pushed a commit to golang/sys that referenced this issue Oct 3, 2021
@tklauser
unix: enable Sysv shared memory support on darwin/arm64
b1ebd4e 
Keep it disabled on ios though.

For golang/go#45696
For golang/go#46084

Change-Id: I3d551227a4ebc0eebabdd16b175aa6a75ea9de19
Reviewed-on: https://go-review.googlesource.com/c/sys/+/353509
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Ian Lance Taylor <iant@golang.org>
@gopherbot
Copy link

Change https://golang.org/cl/354809 mentions this issue: nettest: simplify iOS detection in TestableNetwork

gopherbot pushed a commit to golang/net that referenced this issue Oct 8, 2021
@tklauser
nettest: simplify iOS detection in TestableNetwork
3b03d30 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

Now that Go 1.16 is the oldest supported release, the ios tag can be
used exclusively in TestableNetwork to detect iOS, increasing test
coverage for darwin/arm64.

For golang/go#45696

Change-Id: If607ff8847ed42160ef5e8260d2a8c21798a164a
Reviewed-on: https://go-review.googlesource.com/c/net/+/354809
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@gopherbot
Copy link

Change https://golang.org/cl/358254 mentions this issue: unix: use default directories in TestGetwd on darwin/arm64

gopherbot pushed a commit to golang/sys that referenced this issue Oct 25, 2021
@tklauser
unix: use default directories in TestGetwd on darwin/arm64
69cdffd 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

Now that Go 1.16 is the oldest supported release, the ios tag can be
used exclusively to detect iOS and TestGetwd can use the same list of
directories as other systems.

For golang/go#45696

Change-Id: Ic334df5ea88ac034a9d9271f6cd570617f208f05
Reviewed-on: https://go-review.googlesource.com/c/sys/+/358254
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Go Bot <gobot@golang.org>
Reviewed-by: Cherry Mui <cherryyz@google.com>
@tklauser
Copy link
Member Author

tklauser commented Nov 9, 2021

Some examples:

https://go.googlesource.com/sys/+/33663a62ff0820461192226283cafe57a83ac6b7/unix/syscall_unix_test.go#169 https://go.googlesource.com/sys/+/33663a62ff0820461192226283cafe57a83ac6b7/unix/syscall_unix_test.go#491 https://go.googlesource.com/net/+/4e50805a0758a198d5c9ea96b2e8dacd7338f2ee/nettest/nettest.go#102 https://go.googlesource.com/term/+/f5beecf764ed751c785fa21705385df593b1852d/terminal_test.go#410

These and a few more instances were fixed in the CLs linked by GopherBot. I think this issue can be closed.

@tklauser tklauser closed this as completed Nov 9, 2021
@gopherbot
Copy link

Change https://golang.org/cl/373359 mentions this issue: os: enable TestMkdirAllWithSymlink on darwin/arm64

gopherbot pushed a commit that referenced this issue Jan 2, 2022
@tklauser
os: enable TestMkdirAllWithSymlink on darwin/arm64
d181885 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

TestMkdirAllWithSymlink ought to run on darwin/arm64, so enable it on
that platform.

For #45696

Change-Id: I2cad6b1dfddf215e6b6cd262bbd22251f48f3d8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/373359
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
jproberts pushed a commit to jproberts/go that referenced this issue Jun 21, 2022
@tklauser @jproberts
os: enable TestMkdirAllWithSymlink on darwin/arm64
3427d1a 
Go 1.16 renamed the iOS port from darwin/arm64 to ios/arm64 and
darwin/arm64 was repurposed for the macOS ARM64 port (see
https://golang.org/doc/go1.16#darwin).

TestMkdirAllWithSymlink ought to run on darwin/arm64, so enable it on
that platform.

For golang#45696

Change-Id: I2cad6b1dfddf215e6b6cd262bbd22251f48f3d8c
Reviewed-on: https://go-review.googlesource.com/c/go/+/373359
Trust: Tobias Klauser <tobias.klauser@gmail.com>
Run-TryBot: Tobias Klauser <tobias.klauser@gmail.com>
TryBot-Result: Gopher Robot <gobot@golang.org>
Reviewed-by: Emmanuel Odeke <emmanuel@orijtech.com>
@golang golang locked and limited conversation to collaborators Dec 21, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. OS-Darwin
Projects
None yet
Milestone
Go1.18
Development

No branches or pull requests
3 participants
@tklauser @gopherbot @anton-kuklin