Go: x/sys/windows: API break in SecurityAttributes struct

[pete-woods]

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

$ go version
go version go1.11.13 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

N/A

What did you do?

In this revision of x/sys, there was a breaking API change:
https://github.com/golang/sys/commit/5c00192e8ce6429d705e1e4d83b4b22e79482964#diff-c4ffa695b270239c949ef5b31be6c4f5

This means that Docker doesn't compile any more:
https://github.com/moby/moby/blob/master/pkg/system/filesys_windows.go#L112

What did you expect to see?

Successful compilation.

What did you see instead?

cannot use uintptr(unsafe.Pointer(&sd[0])) (type uintptr) as type *"golang.org/x/sys/windows".SECURITY_DESCRIPTOR in assignment

[ALTree]

cc @alexbrainman @zx2c4

[zx2c4]

Change is intentional. Going from an unsafe member put there as a stop-gap to a properly typed one now that we have support is sensible. x/sys/windows is unversioned in order to allow for these gradual improvements over time.

Fixed here for moby: https://github.com/moby/moby/pull/40017 --> https://github.com/moby/moby/pull/40021

[ianlancetaylor]

The change is intentional but we don't want to break programs like Docker. Can we keep the old struct and define a new one with a different name?

[ianlancetaylor]

CC @tklauser @bradfitz

[zx2c4]

@ianlancetaylor Then we'd have to change every place that actually uses that struct. The cascading effects aren't pretty.

Meanwhile I've already opened a CL to fix this for Docker and things seem to be moving along nicely there.

[alexbrainman]

I agree with @zx2c4

The change is intentional but we don't want to break programs like Docker.

I though we built modules just for such purpose. Lets see if modules work. I don't use modules myself, so I would not know. But maybe we need to bump module version or something, because it is breaking change. Maybe others will help.

Can we keep the old struct and define a new one with a different name?

We changed SecurityAttributes, and it is used as a parameter in 11 exported functions. Do we duplicate all these API?

We changed from

type SecurityAttributes struct {    
    Length             uint32   
    SecurityDescriptor uintptr  
    InheritHandle      uint32   
}

to

type SecurityAttributes struct {
    Length             uint32
    SecurityDescriptor *SECURITY_DESCRIPTOR
    InheritHandle      uint32
}

When we created original version many years ago, we didn't care about use of SecurityDescriptor field. But now people want to use it.

If we cannot change SecurityAttributes, then people like Jason would have to store code in his private repo, and then others cannot benefit from his efforts.

Alex

[tklauser]

In principle, I'd also prefer a new type being introduced. But since the change was already submitted and we have modules to select a particular revision of the module, I'd also be fine to keep it as is. Also, x/sys/windowsis vendored in the case of moby/docker, so the breakage would only occur after vendoring a new version. This is being done along with the necessary API changes in moby/moby#40021.

People who want the old type could also still use syscall.SecurityAttributes.

[ianlancetaylor]

Modules only help if we tag the sources appropriately, which we aren't doing. And a breaking change requires changing the import path to golang.org/x/sys/windows/v2, which seems undesirable for a minor change like this.

I guess if this is underway we can continue. But let's please pay more attention to breaking changes.

[bradfitz]

Modules only help if we tag the sources appropriately

That's not entirely true. If they have a go.mod file, they'll keep getting the same version until they update their deps or update. So the breakage won't appear out of nowhere at least.

[pete-woods]

What happened to us, (and caused us to notice this break), was doing:

• go get -u google.golang.org/grpc
• go get -u github.com/golang/protobuf

then suddenly Docker stopped compiling, as x/sys/windows got updated. Then we had to search the history to find a version Docker still compiled against.

go get -u golang.org/x/sys@acfa387b8d69adbeab4af0736737d42b9f2e8254

It wasn't the end of the world, but pretty inconvenient.

[bradfitz]

It wasn't the end of the world, but pretty inconvenient.

FWIW, I agree we shouldn't be changing the API within a major version. I was just pointing out that it's not as bad as it used to be where different users could get different results depending on when they cloned various stuff.

[jstarks]

Is there an official position on the compatibility guarantees for x/sys/windows? This is the second breaking change I've been made aware of in the last week.

[bradfitz]

Is there an official position on the compatibility guarantees for x/sys/windows? This is the second breaking change I've been made aware of in the last week.

The official position remains the same for all the Go repos: we shouldn't break compatibility. But we only have automated checks for that in the main "go" repo, so sometimes things slip by.

@jba was working on tooling to detect compatibility breakage for all repos, though. (What's the status of that?)

In the meantime we need to step up our human vigilance until we have automation. And/or roll things back when they happen, like I did the other day with the other issue.

I almost think we should roll this one back too on principle and add an accessor method(s) on the type to get at the pointer if we need it.

[zx2c4]

I almost think we should roll this one back too on principle and add an accessor method(s) on the type to get at the pointer if we need it.

Please don't. This change is already in effect, accessor methods are ugly, and a setter method would make clean & readable declarations of the code difficult.

If x/sys/windows can _never_ be broken, I'm not sure that's a worthwhile project for me to continue to contribute to, and I'll probably wind up building something private instead. x/sys/windows is a good start, but there are a lot of weird warts and incomplete things, like this uintptr member, which over time need to be gradually fixed. Windows APIs are complicated and strange enough that it's unlikely we'll get everything perfect on the first try, and evolution is just simply necessary.

[bradfitz]

If x/sys/windows can never be broken, I'm not sure that's a worthwhile project for me to continue to contribute to, and I'll probably wind up building something private instead.

That's fine if you want to do that. It's more fun and locally optimal to hack away by yourself.

But we're building something for many people, and that means not breaking compatibility.

Maybe there's a middle road, though: you could work on a new major version (golang.org/x/sys/windows/v2) and clean up a bunch of stuff all at once.

[alexbrainman]

I almost think we should roll this one back too on principle and add an accessor method(s) on the type to get at the pointer if we need it.

I have no problem with that.

But we also have to consider @zx2c4 sentiment.

Can we create new Git branch in that repo where we can develop. And copy branch changes onto master every once in a while (including incrementing module version number)?

Alex

[zx2c4]

But we're building something for many people, and that means not breaking compatibility.

I thought this is what modules were meant for, and x/sys is still unversioned. @ianlancetaylor wrote in https://github.com/golang/go/issues/34309#issuecomment-531630445 :

"We can break compatibility in x/sys/windows, but I don't think we can break it to that extent."

It seems like this is exactly the kind of "tiny breakage" that ought to be acceptable to keep the project at least minimally healthy. An ironclad commitment to never breaking it, when things are in such flux, will result in something monstrous over time.

[pete-woods]

Seems like this issue has stalled, and the Docker folks are waiting on a decision here: https://github.com/moby/moby/pull/40021#issuecomment-537066824

[zx2c4]

It's been almost two weeks since that commit landed, and various projects have been updated. Seems disruptive to revert and cause _more_ breakage.

[armandgrillet]

It's been more than a month that https://github.com/golang/sys/commit/5c00192e8ce6429d705e1e4d83b4b22e79482964#diff-c4ffa695b270239c949ef5b31be6c4f5 landed and this discussion seems stale, can an owner confirm that the change will not get reverted and close this PR? This will hopefully allow https://github.com/moby/moby/pull/40021 to move forward.

[bradfitz]

Okay, decision: we won't revert. It's not a great situation, but rolling back would just cause different people pain (for the second time), so let's just keep it as it made the API better, even if it did so in an unfortunate way. We'll try to be more careful and maybe start using the new go release / gorelease (#26420) to catch these things like we do for std.

Plus people using modules don't feel the pain until they update deps, so it used to be worse.

Sorry.