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

x/sys/windows: Some types should be available for non-Windows GOOS targets #36485

Open
dcormier opened this issue Jan 9, 2020 · 16 comments · May be fixed by golang/sys#52
Open

x/sys/windows: Some types should be available for non-Windows GOOS targets #36485

dcormier opened this issue Jan 9, 2020 · 16 comments · May be fixed by golang/sys#52

Comments

@dcormier
Copy link
Contributor

@dcormier dcormier commented Jan 9, 2020

What version of Go are you using (go version)?

$ go version
go version go1.13.5 darwin/amd64

Does this issue reproduce with the latest release?

Yes.

What operating system and processor architecture are you using (go env)?

go env Output
$ go env
GO111MODULE=""
GOARCH="amd64"
GOBIN=""
GOCACHE="/Users/[snip]/Library/Caches/go-build"
GOENV="/Users/[snip]/Library/Application Support/go/env"
GOEXE=""
GOFLAGS=""
GOHOSTARCH="amd64"
GOHOSTOS="darwin"
GONOPROXY="[snip]"
GONOSUMDB="[snip]"
GOOS="darwin"
GOPATH="/Users/[snip]/go"
GOPRIVATE="[snip]"
GOPROXY="https://proxy.golang.org,direct"
GOROOT="/usr/local/Cellar/go/1.13.5/libexec"
GOSUMDB="sum.golang.org"
GOTMPDIR=""
GOTOOLDIR="/usr/local/Cellar/go/1.13.5/libexec/pkg/tool/darwin_amd64"
GCCGO="gccgo"
AR="ar"
CC="clang"
CXX="clang++"
CGO_ENABLED="1"
GOMOD=""
CGO_CFLAGS="-g -O2"
CGO_CPPFLAGS=""
CGO_CXXFLAGS="-g -O2"
CGO_FFLAGS="-g -O2"
CGO_LDFLAGS="-g -O2"
PKG_CONFIG="pkg-config"
GOGCCFLAGS="-fPIC -m64 -pthread -fno-caret-diagnostics -Qunused-arguments -fmessage-length=0 -fdebug-prefix-map=/var/folders/84/_6l41bt970l9fmsrwc_p1gv00000gn/T/go-build550252566=/tmp/go-build -gno-record-gcc-switches -fno-common"

What did you do?

Attempt to use golang.org/x/sys/windows.GUID on a non-Windows OS. If you're not running Windows, this will do it:

package main

import "github.com/Microsoft/go-winio/pkg/guid"

func main() {
	guid.ParseString("not a guid")
}

Assuming that file is saved as main.go, run: go run main.go.

(github.com/Microsoft/go-winio/pkg/guid.GUID is defined as golang.org/x/sys/windows.GUID)

What did you expect to see?

The command would run and exit successfully, without an error or other message.

What did you see instead?

This compile error:

[snip]/github.com/Microsoft/go-winio/pkg/guid/guid.go:16:2: build constraints exclude all Go files in [snip]/golang.org/x/sys/windows

Additional information

I was attempting to use this to create something to interface with ActiveDirectory (or LDAP), but won't be running on Windows. There are common values in ActiveDirectory that are GUIDs, so it would be very helpful to be able to use this type. Especially in combination with Microsoft's package that uses that type.

@gopherbot gopherbot added this to the Unreleased milestone Jan 9, 2020
@dcormier dcormier linked a pull request that will close this issue Jan 9, 2020
@gopherbot

This comment has been minimized.

Copy link

@gopherbot gopherbot commented Jan 9, 2020

Change https://golang.org/cl/214177 mentions this issue: windows: Made GUID type available to other OS's

@toothrot

This comment has been minimized.

Copy link
Contributor

@toothrot toothrot commented Jan 9, 2020

@bradfitz

This comment has been minimized.

Copy link
Contributor

@bradfitz bradfitz commented Jan 9, 2020

I'm not sure we want to do this.

Seems like github.com/Microsoft/go-winio/pkg/guid can redefine those if it wants to be portable?

I'm not entirely opposed to this, but I don't know where this ends.

/cc @ianlancetaylor @tklauser

@ianlancetaylor

This comment has been minimized.

Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Jan 10, 2020

I'm inclined to agree that this should be a separate package. We shouldn't have to try to support golang.org/x/sys/windows on non-Windows systems.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 10, 2020

I'm not sure we want to do this.

I agree. x/sys/windows is started as Windows version of syscall package. And syscall.GUID makes no sense anywhere but Windows.

Alex

@tklauser

This comment has been minimized.

Copy link
Member

@tklauser tklauser commented Jan 10, 2020

I tend to agree with previous comments that we shouldn't try to make x/sys/windows support non-Windows systems. I think this should either go into its own package or be duplicated in github.com/Microsoft/go-winio/pkg/guid.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jan 10, 2020

Has an open issue in go-winio: microsoft/go-winio#158

@dcormier

This comment has been minimized.

Copy link
Contributor Author

@dcormier dcormier commented Jan 10, 2020

We shouldn't have to try to support golang.org/x/sys/windows on non-Windows systems.

While I'm asking for windows.GUID to be accessible (whether just exposed in its current package or moved), there are likely other types, vars, and consts in that package that would probably be useful when trying to interoperate with Microsoft's products from other OS's. It wouldn't surprise me if the same is true for other GOOS targets under x/sys/.

Maybe it's worthwhile to consider exposing at least some types, vars, and consts from the packages under x/sys/*/? I'm not referring to any functions that would actually require syscalls, obviously.

For me, personally, this is not the first time in my career that I've needed to deal with structures that are common on one OS from another OS where they are not common. I'm sure it won't be the last.

And syscall.GUID makes no sense anywhere but Windows.

Unless you're writing something that needs to be able to deal with GUIDs, and doesn't run on Windows. Case in point, the thing I'm writing that caused me to open this issue in the first place. It needs to interoperate with Active Directory, but it doesn't run on Windows.

I'm not advocating that GUID live in syscall, just somewhere where it can be used in programs not built for Windows.

Has an open issue in go-winio: microsoft/go-winio#158

More evidence that sometimes people need to deal with GUIDs while not running on Windows.

@networkimprov

This comment has been minimized.

Copy link

@networkimprov networkimprov commented Jan 10, 2020

The stdlib or x/ should have LDAP & AD support since Go is oriented to network apps...

Have you seen https://github.com/korylprince/go-ad-auth?

Go-Winio would likely take a pull request to drop the x/sys/windows dependency.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 10, 2020

Maybe it's worthwhile to consider exposing at least some types, vars, and consts from the packages under x/sys/*/?

Sure. What do you suggest?

And, do not forget, this is open source code, so you can copy whatever you like and put it anywhere you like.

I'm not referring to any functions that would actually require syscalls, obviously.

At lease you agree with me on this point.

Then I consider windows.GUID should only be useful to call actual syscalls. It should not be used for anything else. If you want to use windows.GUID for anything else, you need different type. It is fine, if you copy the code.

Alex

dcormier added a commit to dcormier/go-winio that referenced this issue Jan 13, 2020
`golang.org/x/sys/windows.GUID` is currently only available to builds
targeting `GOOS=windows` (see golang/go#36485). This change makes it
so `windows` builds continue to use `golang.org/x/sys/windows.GUID`,
while non-`windows` builds get a different structure that has all
the same fields and methods.
@dcormier

This comment has been minimized.

Copy link
Contributor Author

@dcormier dcormier commented Jan 13, 2020

Sure. What do you suggest?

Honestly, exposing everything that doesn't rely on making a syscall. Structs, vars, and consts. The only things that shouldn't be exposed are functions and methods that make syscalls.

And, do not forget, this is open source code, so you can copy whatever you like and put it anywhere you like.

Of course. And that is what I have to do to continue my work at the moment. But since the code already exists in x/sys, it would be nice if it could just be used.

Then I consider windows.GUID should only be useful to call actual syscalls.

It's more useful than that, though. For example, what I ran into that resulted in opening this issue in the first place. I'm not making syscalls, I'm interacting with ActiveDirectory (Microsoft's LDAP implementation) over a network. There are GUIDs in there that I need to handle. The existing windows.GUID struct appears to be the correct type to use.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 15, 2020

There are GUIDs in there that I need to handle. The existing windows.GUID struct appears to be the correct type to use.

Can you, please, show small code that demonstrates your point. How exactly will you use the struct? What will you program do?

Alex

@dcormier

This comment has been minimized.

Copy link
Contributor Author

@dcormier dcormier commented Jan 15, 2020

I can't just pull that code from this project, unfortunately. But the gist of it is:

// `entry` comes from an LDAP search, which there is an example of here:
// https://pkg.go.dev/github.com/go-ldap/ldap/v3@v3.1.5?tab=doc#example-Conn.Search
// It's an instance of:
// https://pkg.go.dev/github.com/go-ldap/ldap/v3@v3.1.5?tab=doc#Entry

var guidValue [16]byte
copy(guidValue[:], entry.GetRawAttributeValue("objectGUID"))

// https://pkg.go.dev/github.com/Microsoft/go-winio@v0.4.14/pkg/guid?tab=doc#FromWindowsArray
obejctGUID := guid.FromWindowsArray(guidValue)

// The value of `objectGUID.String()` ends up written out to
// JSON that contains representations of user objects from
// LDAP. That's then sent to another system.
@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 19, 2020

But the gist of it is:

Good enough for me.

obejctGUID := guid.FromWindowsArray(guidValue)

ToWindowsArray

// ToWindowsArray returns an array of 16 bytes representing the GUID in Windows
// encoding.
func (g GUID) ToWindowsArray() [16]byte {
	return g.toArray(binary.LittleEndian)
}

https://github.com/microsoft/go-winio/blob/6c72808b55902eae4c5943626030429ff20f3b63/pkg/guid/guid.go#L120-L124

uses binary.LittleEndian. So, even if we make golang.org/x/sys/windows.GUID available on all platforms supported by Go, ToWindowsArray function will be broken everywhere where binary.LittleEndian is not default encoding.

So you still need to talk to the owners of github.com/Microsoft/go-winio/pkg/guid package, if you want to use it on non-Windows.

Alex

@dcormier

This comment has been minimized.

Copy link
Contributor Author

@dcormier dcormier commented Jan 20, 2020

https://github.com/microsoft/go-winio/blob/6c72808b55902eae4c5943626030429ff20f3b63/pkg/guid/guid.go#L120-L124

uses binary.LittleEndian. So, even if we make golang.org/x/sys/windows.GUID available on all platforms supported by Go, ToWindowsArray function will be broken everywhere where binary.LittleEndian is not default encoding.

So you still need to talk to the owners of github.com/Microsoft/go-winio/pkg/guid package, if you want to use it on non-Windows.

With the sample I gave, this will work just fine on big-endian platforms as the GUID stored in that LDAP attribute is in little-endian. The endianness of the platform you're accessing LDAP from doesn't matter. The bytes in that attribute are always going to be in the same order. So the function to read those bytes into a GUID also needs to always use the same order.

Since guid.FromWindowsArray explicitly handles the bytes being in that order, the sample code I gave would work correctly, regardless of the endianness of the platform the code is running on. Similarly, the (guid.GUID).ToWindowsArray() function will always return an array of bytes with that same order.

There is a separate function for times when you have a GUID stored in big-endian, and a separate method when you need a GUID turned into a big-endian byte array.

@alexbrainman

This comment has been minimized.

Copy link
Member

@alexbrainman alexbrainman commented Jan 23, 2020

With the sample I gave, this will work just fine on big-endian platforms as the GUID stored in that LDAP attribute is in little-endian.

You are correct here. I was wrong with my example.

I am still against making any of golang.org/x/sys/windows available on non Windows. But I won't stop others.

Alex

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

8 participants
You can’t perform that action at this time.