search

moby / term

Apache License 2.0
66 stars 27 forks source link

Fix build for windows/386 platform #14

Closed saschagrunert closed 3 years ago

saschagrunert commented 4 years ago

Before this patch, the build on 32bit windows would fail with:

> GOOS=windows GOARCH=386 go build ./...
./term_windows.go:74:39: constant 4294967286 overflows int
./term_windows.go:80:40: constant 4294967285 overflows int
./term_windows.go:86:40: constant 4294967284 overflows int

We now cast the values to int64 to avoid this error.

thaJeztah commented 4 years ago

Also curious; is windows/386 still a thing (are there packages building for that?) or did you run into this on an over-enthusiastic CI matrix?

saschagrunert commented 4 years ago

Also curious; is windows/386 still a thing (are there packages building for that?) or did you run into this on an over-enthusiastic CI matrix?

Yeah, we're building cri-tools for windows/386. Go 1.15 dropped support for darwin/386 so we could also consider dropping that binary too. The main import was coming from the docker/distribution package.

thaJeztah commented 4 years ago

Yeah, we're building cri-tools for windows/386. Go 1.15 dropped support for darwin/386 so we could also consider dropping that binary too. The main import was coming from the docker/distribution package.

Thanks for the extra info!

I had a quick check internally as well wether windows/386 still should be considered "a thing", and apparently it's still used, but mostly in VMs to run legacy software, so I doubt those machines would have docker or kubernetes installed (famous last words 😂)

So, yes, I'm usually ok with changes like this (doesn't really hurt to at least make it build), just a bit cautious on the breaking change.

That said, this repository has not done a tagged release yet, so "technically" it's ok to still be "unstable". The breaking change likely wouldn't be an issue for k8s / bigger projects that more likely pin dependencies, but may trip over other users.

saschagrunert commented 4 years ago

That said, this repository has not done a tagged release yet, so "technically" it's ok to still be "unstable". The breaking change likely wouldn't be an issue for k8s / bigger projects that more likely pin dependencies, but may trip over other users.

Hm I see, I think it just depends how we wanna move forward with this change.

thaJeztah commented 3 years ago

@saschagrunert I think this issue may be resolved by https://github.com/moby/term/pull/24; would you be able to test/confirm?

saschagrunert commented 3 years ago

@saschagrunert I think this issue may be resolved by #24; would you be able to test/confirm?

Yep this works, thank you!

thaJeztah commented 3 years ago

Perfect, thanks!!