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: API break in SecurityAttributes struct #34610

Closed
pete-woods opened this issue Sep 30, 2019 · 21 comments
Closed

x/sys/windows: API break in SecurityAttributes struct #34610

pete-woods opened this issue Sep 30, 2019 · 21 comments

Comments

@pete-woods
Copy link

@pete-woods pete-woods commented Sep 30, 2019

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:
golang/sys@5c00192#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 ALTree changed the title API break in `golang.org/x/sys` x/sys/windows: API break in SecurityAttributes struct Sep 30, 2019
@gopherbot gopherbot added this to the Unreleased milestone Sep 30, 2019
@ALTree
Copy link
Member

@ALTree ALTree commented Sep 30, 2019

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Sep 30, 2019

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: moby/moby#40017 --> moby/moby#40021

@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 1, 2019

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 ianlancetaylor modified the milestones: Unreleased, Go1.14 Oct 1, 2019
@ianlancetaylor
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 1, 2019

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Oct 1, 2019

@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
Copy link
Member

@alexbrainman alexbrainman commented Oct 1, 2019

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
Copy link
Member

@tklauser tklauser commented Oct 1, 2019

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
Copy link
Contributor

@ianlancetaylor ianlancetaylor commented Oct 1, 2019

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
Copy link
Contributor

@bradfitz bradfitz commented Oct 1, 2019

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
Copy link
Author

@pete-woods pete-woods commented Oct 1, 2019

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
Copy link
Contributor

@bradfitz bradfitz commented Oct 1, 2019

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
Copy link

@jstarks jstarks commented Oct 1, 2019

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
Copy link
Contributor

@bradfitz bradfitz commented Oct 1, 2019

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
Copy link
Contributor

@zx2c4 zx2c4 commented Oct 1, 2019

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
Copy link
Contributor

@bradfitz bradfitz commented Oct 1, 2019

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
Copy link
Member

@alexbrainman alexbrainman commented Oct 1, 2019

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
Copy link
Contributor

@zx2c4 zx2c4 commented Oct 1, 2019

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 #34309 (comment) :

"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
Copy link
Author

@pete-woods pete-woods commented Oct 3, 2019

Seems like this issue has stalled, and the Docker folks are waiting on a decision here: moby/moby#40021 (comment)

@zx2c4
Copy link
Contributor

@zx2c4 zx2c4 commented Oct 3, 2019

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
Copy link

@armandgrillet armandgrillet commented Nov 7, 2019

It's been more than a month that golang/sys@5c00192#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 moby/moby#40021 to move forward.

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 7, 2019

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.

@bradfitz bradfitz closed this Nov 7, 2019
carolynvs added a commit to carolynvs/cnab-go that referenced this issue Jan 6, 2020
Upstream golang.org/x/sys introduced a breaking change within a major
version again, see golang/go#34610. The
workaround is to pin it to the last revevion that is compatible with
moby/moby.
carolynvs added a commit to carolynvs/cnab-go that referenced this issue Jan 6, 2020
Upstream golang.org/x/sys introduced a breaking change within a major
version, see golang/go#34610. The
workaround is to pin it to the last revevion that is compatible with
the version of moby/moby that we are using.
carolynvs added a commit to carolynvs/cnab-go that referenced this issue Jan 6, 2020
Upstream golang.org/x/sys introduced a breaking change within a major
version, see golang/go#34610. The
workaround is to pin it to the last revesion that is compatible with
the version of moby/moby that we are using.

Verify that the code compiles on windows, macos and linux during our CI
run to catch problems like this one (a regression in golang.org/x/sys)
in the future as soon as it happens.
carolynvs added a commit to carolynvs/cnab-go that referenced this issue Jan 6, 2020
Upstream golang.org/x/sys introduced a breaking change within a major
version, see golang/go#34610. The
workaround is to pin it to the last revesion that is compatible with
the version of moby/moby that we are using.

Verify that the code compiles on windows, macos and linux during our CI
run to catch problems like this one (a regression in golang.org/x/sys)
in the future as soon as it happens.

Signed-off-by: Carolyn Van Slyck <carolyn.vanslyck@microsoft.com>
carolynvs added a commit to carolynvs/cnab-go that referenced this issue Jan 6, 2020
Upstream golang.org/x/sys introduced a breaking change within a major
version, see golang/go#34610. The
workaround is to pin it to the last revesion that is compatible with
the version of moby/moby that we are using.

Verify that the code compiles on windows, macos and linux during our CI
run to catch problems like this one (a regression in golang.org/x/sys)
in the future as soon as it happens.

Also do a reset of our go dependencies after bootstrap. When we use go
get to install global dependencies, it modifies our go.* files changing
the versions of the dependencies that we required, making the build
break, because kind requires different versions than docker. This resets
it back to what we need before we compile.

Signed-off-by: Carolyn Van Slyck <carolyn.vanslyck@microsoft.com>
mdelapenya added a commit to mdelapenya/testcontainers-go that referenced this issue Aug 24, 2020
pojntfx added a commit to pojntfx/alpimager that referenced this issue Aug 24, 2020
@kasey kasey mentioned this issue Sep 28, 2020
2 of 2 tasks complete
jadolg added a commit to jadolg/DockerImageSave that referenced this issue Oct 1, 2020
See here for more context golang/go#34610
fho pushed a commit to simplesurance/baur that referenced this issue Oct 21, 2020
This change set addresses #94 and ports baur to Windows. The `make release` command will create a _baur.exe_ file.
Things to note:
1. A [breaking change](golang/go#34610) was introduced to golang.org/x/sys that prevents github.com/docker/docker from compiling. This issue in github.com/docker/docker will not be [fixed](moby/moby#40021) until v20 of github.com/docker/docker is released. In the meantime I have pinned golang.org/x/sys to the latest version before the breaking change was introduced.
2. Some of the existing tests used commands specific to Unix. These have either been replaced with equivalent commands available directly on both Unix and Windows or run via `cmd /C` on Windows.
3. Windows does not allow files that are in use to be deleted in the same way as Unix. In some tests files and folders had to be released so the test cleanup succeeds.
4. Expected paths have been updated to use OS specific path separators where required.
5. CircleCI does not provide Windows containers but does provide Windows VMs
    * The pre-installed version of golang is 1.12.7, this needs to be updated on every build. See [software-pre-installed-in-the-windows-image](https://circleci.com/docs/2.0/hello-world-windows/#software-pre-installed-in-the-windows-image)
    * I could not access Unix docker containers from within the Windows job so postgres is installed on the VM via chocolatey and the default "postgres" database is used.
    * The tests are much slower to run on Windows than Unix, the test timeout has been increased to accommodate this.
    * The `-race` flag has been removed from the Windows `go test` command as a gcc compiler is not installed. I figured this wasn't a problem as it is used in the existing test job. This could be addressed later if necessary.
@golang golang locked and limited conversation to collaborators Nov 6, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
You can’t perform that action at this time.