zeroshade commented on PR #103: URL: https://github.com/apache/terraform-provider-iceberg/pull/103#issuecomment-5442531966
Pushed f162fb1. Not from a review comment — it came out of chasing @dttung2905's GOROOT report — but it changes what this PR is asserting, so it's worth flagging before merge. **The version pin wasn't being honored.** `ensure_go_licenses` preferred any `go-licenses` on `PATH` ahead of `GO_LICENSES_VERSION`. On my machine that meant every run of this script, including the ones that produced the committed `LICENSE-binary`, used my own `~/go/bin/go-licenses` rather than the pinned v1.6.0. That matters because versions disagree about which file in a module *is* its license: | module | v1.6.0 reads | v2.0.1 reads | | --- | --- | --- | | `github.com/apache/thrift` | `lib/go/README.md` | `LICENSE` | | `github.com/apache/iceberg-go` | `catalog/README.md` | *(correctly ignored)* | v1.6.0 is matching **README files** as license files. It lands on Apache-2.0 for both, so the committed inventory is correct — but by accident rather than by detection. v2.0.1 reads thrift's real `LICENSE`, which is a composite, and reports three classifications from it: Apache-2.0, BSD-3-Clause, and GNU-All-permissive. Those appendix sections cover ``` aclocal/ax_boost_base.m4 lib/cpp/src/thrift/windows/SocketPair.cpp compiler/cpp/.../sha256.h lib/erl/src/Makefile.am contrib/fb303/aclocal/... lib/nodejs/lib/thrift/json_parse.js doc/otp-base-license.txt lib/py/compat/win32/stdint.h ``` — nothing under `lib/go`, so Apache-2.0 remains the right answer for what we actually link. Under v2 it would need an override recording that explicitly. f162fb1 drops the `PATH` lookup and puts the version in the install path (`build/tools/go-licenses-v1.6.0/`), so the pin is real and moving it installs afresh instead of reusing an older binary. I re-ran the full 8-platform `--check` against the pinned binary: `LICENSE-binary` and `licenses-binary/` match, so what is committed here is correct under v1.6.0 specifically, not merely under whatever I had installed. I deliberately did **not** bump to v2 here — it moves classifications and wants the inventory re-reviewed, which is not what should be landing in a release-blocking change. Good candidate for the follow-up issues. <details> <summary>Why not pin with a go.mod <code>tool</code> directive</summary> Worth recording, since `go get -tool` is the modern answer and I tried it: - `go get -tool github.com/google/[email protected]` **breaks `go mod tidy`**. It drags in `go.opencensus.io` → `cloud.google.com/go/compute v1.7.0`, which predates the `metadata` module split, so tidy fails with an ambiguous import for `cloud.google.com/go/compute/metadata`. Tidy is clean today, so this would be a regression in the released module. go.sum also grows 732 → 1416 lines. - v2.0.1 as a tool directive *is* clean — tidy passes, go.sum grows only to 827, and the linked set is byte-identical — so this becomes viable if we ever bump. - Either way it could not be invoked as `go tool go-licenses`: `collect()` sets `GOOS`/`GOARCH` to cross-analyze each platform and `go tool` obeys them, building a foreign binary and failing to exec it (`exec format error`). It would have to be `go tool -n` to get a host binary path. </details> -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected] --------------------------------------------------------------------- To unsubscribe, e-mail: [email protected] For additional commands, e-mail: [email protected]
