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

proposal: runtime: add custom types and constants for GOOS, GOARCH #64046

Open
Nasfame opened this issue Nov 9, 2023 · 16 comments
Open

proposal: runtime: add custom types and constants for GOOS, GOARCH #64046

Nasfame opened this issue Nov 9, 2023 · 16 comments
Labels
Milestone

Comments

@Nasfame
Copy link
Contributor

Nasfame commented Nov 9, 2023

Proposal: Add custom types and constants for GOOS, GOARCH

Motivation

The current GOOS and GOARCH variables are strings, making it difficult to write platform-specific code. Adding custom types and constants would make it easier and more reliable.

Design

Add the GOOS, GOARCH types and constants to the runtime package:

go tool dist list
aix/ppc64
android/386
android/amd64
android/arm
android/arm64
darwin/amd64
darwin/arm64
dragonfly/amd64
freebsd/386
freebsd/amd64
freebsd/arm
freebsd/arm64
freebsd/riscv64
illumos/amd64
ios/amd64
ios/arm64
js/wasm
linux/386
linux/amd64
linux/arm
linux/arm64
linux/loong64
linux/mips
linux/mips64
linux/mips64le
linux/mipsle
linux/ppc64
linux/ppc64le
linux/riscv64
linux/s390x
netbsd/386
netbsd/amd64
netbsd/arm
netbsd/arm64
openbsd/386
openbsd/amd64
openbsd/arm
openbsd/arm64
plan9/386
plan9/amd64
plan9/arm
solaris/amd64
wasip1/wasm
windows/386
windows/amd64
windows/arm
windows/arm64

Prototype

Proposed Implementation for GOOS: https://go.dev/play/p/iXj3bRkQxwL

  • If there exists constants for OS, can we link them in runtime package like time.Sleep??

Benefits

  1. Make it easier to write platform-specific code.
  2. Improve the readability and maintainability of platform-specific code.
  3. Reduce the number of errors in platform-specific code.
  4. Reduce magic values like in runtime.GOOS=="windows"

Backwards Compatibility

To ensure backwards compatibility:

  1. fork src/runtime/extern.go to exter_go1.21.go or use build directives. Currently none employed, so no complications.
2. use `type GOOS string in Go1.x or until Go1.x releases support for generics in constants and `type GOOS OS` in Go2.x
@gopherbot gopherbot added this to the Proposal milestone Nov 9, 2023
@mateusz834
Copy link
Member

I think that this kind of stuff should be it in a separate package like runtime/goos rather than adding more stuff to runtime.

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

@mateusz834 I don't think its worth it. Its just a couple of constants. Creating a file - goos.go and putting it up there makes more sense.

If u can outline what u want to put over there, it would make more sense to me

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

if runtime.GOOS==runtime.WINDOWS{
}
if runtime.GOOS==goos.WINDOWS{
}

Second is okay, it doesn't bloat runtime. but since GOOS is inside runtime, it makes more sense to keep it inside runtime.

@randall77
Copy link
Contributor

randall77 commented Nov 10, 2023

I don't understand the benefit of doing runtime.GOOS == runtime.WINDOWS instead of runtime.GOOS == "windows". I suppose it lets the compiler catch typos, but it seems unlikely that a mistake like that would be uncaught by any test.

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

@randall77 it makes a huge difference actually.

The Compiler catches the typos and accidental mistakes during refactoring, which could go unnoticed until manually tested or BDD tests are written

Plus i don't think remembering all the 42 pairs is necessary

@mateusz834
Copy link
Member

mateusz834 commented Nov 10, 2023

I don't think its worth it. Its just a couple of constants. Creating a file - goos.go and putting it up there makes more sense.

I don't agree, having constants like Windows, Linux, Plan9 would make it not easy to find all of them through a lsp, if this were to be defined in runtime the constants should be prefixed with some kind of GOOS, like GOOSWindows, so that it is easier to find all OS with gopls, and this is why I would prefer having them in a separate package (GOOSWindows looks weird)

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

like GOOSWindows, so that it is easier to find all OS with gopls, and this is why I would prefer having them in a separate package (GOOSWindows looks weird)

I would prefer: GOOS_WINDOWS over GOOSWindows .

I agree its ok to put it over goos pkg with updated runtime.GOOS doc

@gophun
Copy link

gophun commented Nov 10, 2023

What do you expect to happen to the constant when support for a GOOS or GOARCH is dropped? What about other Go implementations like TinyGo that support a different set of geese and architectures?

@zephyrtronium
Copy link
Contributor

I don't understand the benefit of doing runtime.GOOS == runtime.WINDOWS instead of runtime.GOOS == "windows". I suppose it lets the compiler catch typos, but it seems unlikely that a mistake like that would be uncaught by any test.

FWIW, GOOS and GOARCH comparisons are a bit unique in that they require more infrastructure to actually run tests for them. It reminds me of #20616. Granted it's hard to imagine writing the comparison in the first place if that kind of infrastructure isn't accessible.

That said, this seems like it would be fine as a custom linter, given the rarity of the problem. You could even parse the output of go tool dist list to keep the list in sync with Go version changes. And then you wouldn't have to wait for the proposal process and implementation.

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

What do you expect to happen to the constant when support for a GOOS or GOARCH is dropped? What about other Go implementations like TinyGo that support a different set of geese and architectures?

@gophun i m not sure whether its doable to drop support for GOOS,GOARCH cuz its not backward's compatible

@zephyrtronium
Copy link
Contributor

@Nasfame Multiple GOOS and GOARCH values have already been dropped in Go's history. NaCl and amd64p32 come to mind.

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

@zephyrtronium I see, go toolchain support removed support for different platforms. I get it.. So in that case we can deprecate the constants!!

@gophun
Copy link

gophun commented Nov 10, 2023

Still, what about other Go implementations with different GOOS/GOARCH sets? They can't have different versions of this package; otherwise, a project wouldn't compile with implementation A simply because there's an 'if' check for a platform supported by implementation B but not by A.

My opinion:
If you frequently use specific GOOS/GOARCH values in a project and worry about making numerous typos, you can define the constants yourself, requiring attention only during the initial setup.

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

@gophun the constants are not going to have different versions. No build directives!! . Its like src/runtime/extern.go. They are just constants that are not going to be removed even if go drops support, just deprecated.

GOOS values are defined in go source code. I think it makes more sense to compare with those defined values rather than user defined values

@gophun
Copy link

gophun commented Nov 10, 2023

So, will there be constants for all the GOARCHs that gccgo can target, but gc can't? Like alpha, arc, avr32, blackfix, c6x, cris, frv, h8300, hexagon, ia64, m32r, m68k, metag, microblaze, mn10300, nios2, openrisc, parisc, score, sh, tile, unicore32, vax, xtensa

@Nasfame
Copy link
Contributor Author

Nasfame commented Nov 10, 2023

@gophun The type of GOARCH is still string, so there is still flexibility to compare with all the archs. But I personally think its unnecessary to include architectures and stick the os-arch pairs mentioned in the proposal. If a ticket comes up to include all the architectures supported by gccgo.......

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
Status: Incoming
Development

No branches or pull requests

6 participants