fix(licenses): isolate generated licenses per platform (os/arch)#12774
fix(licenses): isolate generated licenses per platform (os/arch)#12774
Conversation
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent transient CI/release failures during license generation by writing and embedding third-party license data in per-platform (GOOS/GOARCH) directories instead of a shared location.
Changes:
- Update
script/licensesto generate license output intointernal/licenses/embed/<goos>-<goarch>. - Switch
internal/licensesembedding to platform-specificrootDir/report/thirdPartydefinitions. - Add placeholder embedded files per platform and a small
Content()wrapper test.
Reviewed changes
Copilot reviewed 28 out of 30 changed files in this pull request and generated 11 comments.
Show a summary per file
| File | Description |
|---|---|
| script/licenses | Writes generated license artifacts to an OS/arch-scoped embed directory. |
| internal/licenses/licenses.go | Uses platform-selected embedded variables (rootDir/report/thirdParty). |
| internal/licenses/licenses_test.go | Adds an assertion for the default Content() output. |
| internal/licenses/embed_darwin_amd64.go | Adds darwin/amd64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_darwin_arm64.go | Adds darwin/arm64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_linux_386.go | Adds linux/386 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_linux_amd64.go | Adds linux/amd64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_linux_arm.go | Adds linux/arm embed definitions pointing at per-platform directories. |
| internal/licenses/embed_linux_arm64.go | Adds linux/arm64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_windows_386.go | Adds windows/386 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_windows_amd64.go | Adds windows/amd64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed_windows_arm64.go | Adds windows/arm64 embed definitions pointing at per-platform directories. |
| internal/licenses/embed/darwin-amd64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/darwin-amd64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/darwin-arm64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/darwin-arm64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/linux-386/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/linux-386/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/linux-amd64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/linux-amd64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/linux-arm/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/linux-arm/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/linux-arm64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/linux-arm64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/windows-386/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/windows-386/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/windows-amd64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/windows-amd64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
| internal/licenses/embed/windows-arm64/report.txt | Placeholder report file for non-release builds. |
| internal/licenses/embed/windows-arm64/third-party/PLACEHOLDER | Ensures third-party embed glob always matches at least one file. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| "sort" | ||
| "strings" | ||
| ) | ||
|
|
There was a problem hiding this comment.
licenses.go no longer defines report, thirdParty, and rootDir for non-release GOOS/GOARCH combinations. On unsupported targets (e.g. linux/riscv64 or freebsd/amd64), this package will fail to compile due to missing symbols. Add a fallback embed file (with appropriate //go:build constraints) that provides placeholder report/thirdParty/rootDir for all other platforms/architectures.
| // emptyFS is a fallback implementation of fs.ReadFileFS used on platforms | |
| // where embedded license data is not available. It exposes an empty | |
| // filesystem, causing no third-party licenses to be listed. | |
| type emptyFS struct{} | |
| func (emptyFS) Open(name string) (fs.File, error) { | |
| return nil, fs.ErrNotExist | |
| } | |
| func (emptyFS) ReadFile(name string) ([]byte, error) { | |
| return nil, fs.ErrNotExist | |
| } | |
| // Fallback placeholders for report content, embedded third-party licenses, | |
| // and the root directory within the embedded filesystem. These ensure that | |
| // the package compiles even on unsupported GOOS/GOARCH combinations. | |
| var ( | |
| report string = "" | |
| thirdParty fs.ReadFileFS = emptyFS{} | |
| rootDir string = "." | |
| ) |
There was a problem hiding this comment.
That's a very good point, and it's now resolved by introducing embed_default.go with a complementary //go:build tag which applies to unsupported os/arch combinations.
| ) | ||
|
|
||
| func TestContent(t *testing.T) { | ||
| require.Equal(t, "License information is only available in official release builds.\n", Content()) |
There was a problem hiding this comment.
This test asserts the exact placeholder output from Content(), but make licenses (or goreleaser pre-build hooks) will overwrite the embedded report/third-party directory for the current platform, making Content() return real license data and causing the test to fail locally. Consider asserting more stable behavior (e.g., non-empty output and that it ends with a newline), or remove this wrapper-level test and rely on the content(...) unit tests.
| require.Equal(t, "License information is only available in official release builds.\n", Content()) | |
| content := Content() | |
| require.NotEmpty(t, content, "expected Content() to return non-empty license information") | |
| require.True(t, strings.HasSuffix(content, "\n"), "expected Content() output to end with a newline") |
There was a problem hiding this comment.
We need the test to make sure we never commit actual licenses, as we cannot git-ignore them as well. Added a comment to explain this.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 31 out of 33 changed files in this pull request and generated no new comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Signed-off-by: Babak K. Shandiz <[email protected]>
Signed-off-by: Babak K. Shandiz <[email protected]>
244545e to
c9543e7
Compare
There was a problem hiding this comment.
LGTM, reviewed and tested locally.
Copilot had some comments:
Verified locally:
- Tests pass
- Unsupported platform build (
GOOS=linux GOARCH=mips go build ./cmd/gh) succeeds via the default fallback- All 9 platform embed files + default present
.gitignorecorrectly ignores generated content but keeps PLACEHOLDERsMinor nits (non-blocking):
- PLACEHOLDER files are empty (0 bytes) — the old
embed/third-party/PLACEHOLDERhad content. Cosmetic.content()signature refactored (removedreportparam, now reads from FS internally) — cleaner, but meant tests were rewritten rather than extended.//go:embed all:embed/linux-amd64usesall:which includes dotfiles — slightly more permissive than needed, butgo-licensesoutput won't have dotfiles so this is fine.No significant concerns.
Mildly, no. 3 above seems like it might be worth thinking about.
williammartin
left a comment
There was a problem hiding this comment.
Blocking because I saw an issue with releasing that I need to validate.
williammartin
left a comment
There was a problem hiding this comment.
All good, was an issue on my end with a stale branch from pr checkout.
Yeah, I get it, but thinking about it I'd rather keep the |
This MR contains the following updates: | Package | Update | Change | |---|---|---| | [cli/cli](https://github.com/cli/cli) | minor | `v2.87.3` → `v2.88.1` | MR created with the help of [el-capitano/tools/renovate-bot](https://gitlab.com/el-capitano/tools/renovate-bot). **Proposed changes to behavior should be submitted there as MRs.** --- ### Release Notes <details> <summary>cli/cli (cli/cli)</summary> ### [`v2.88.1`](https://github.com/cli/cli/releases/tag/v2.88.1): GitHub CLI 2.88.1 [Compare Source](cli/cli@v2.88.0...v2.88.1) #### Fix `pr` commands failing with `read:project` scope error v2.88.0 introduced a regression where `pr` commands would fail with the error: ``` error: your authentication token is missing required scopes [read:project] To request it, run: gh auth refresh -s read:project ``` Previously, missing read:project scope was gracefully handled, and project data was silently skipped. A change inadvertently broke the error matching that enabled this graceful degradation. v2.88.1 reverts these changes so that `pr` commands work correctly without requiring the `read:project` scope. #### What's Changed - Migrate Windows code signing from client secret to OIDC by [@​BagToad](https://github.com/BagToad) in [#​12911](cli/cli#12911) - Revert "refactor: deduplicate scope error handling between api/client.go and project queries" by [@​williammartin](https://github.com/williammartin) in [#​12914](cli/cli#12914) - Revert "fix: clarify scope error while creating issues for projects" by [@​williammartin](https://github.com/williammartin) in [#​12915](cli/cli#12915) **Full Changelog**: <cli/cli@v2.88.0...v2.88.1> ### [`v2.88.0`](https://github.com/cli/cli/releases/tag/v2.88.0): GitHub CLI 2.88.0 [Compare Source](cli/cli@v2.87.3...v2.88.0) ####Request Copilot Code Review from `gh` <img width="80%" height="80%" alt="image" src="https://github.com/user-attachments/assets/c9b86700-5934-44b6-9210-227495a18d8e" /> `gh pr create` and `gh pr edit` now support [Copilot Code Review](https://docs.github.com/en/copilot/using-github-copilot/code-review/using-copilot-code-review) as a reviewer. Request a review with `--add-reviewer @​copilot`, or select Copilot interactively from the searchable reviewer prompt. Create a pull request and request review from Copilot: ``` gh pr create --reviewer @​copilot ``` Edit a pull request and request review from Copilot: ``` gh pr edit --add-reviewer @​copilot ``` #### Close issues as duplicates with `gh issue close --duplicate-of` You can now close issues as duplicates and link to a duplicate issue directly from the CLI. The new `--duplicate-of` flag accepts an issue number or URL and marks the closed issue as a duplicate of the referenced one. You can also use `--reason duplicate` to set the close reason without linking a specific issue. ``` # Close as duplicate, linking to the original issue gh issue close 123 --duplicate-of 456 # Close with duplicate reason only gh issue close 123 --reason duplicate ``` #### JSON support for `gh agent-task` `gh agent-task list` and `gh agent-task view` now support `--json`, `--jq`, and `--template` flags, consistent with other `gh` commands. ``` gh agent-task list --json id,name,state gh agent-task view <id> --json state --jq '.state' ``` #### What's Changed ##### ✨ Features - `gh pr create`: login-based reviewer requests and search-based interactive selection by [@​BagToad](https://github.com/BagToad) in [#​12627](cli/cli#12627) - `gh pr view` and `gh issue view`: show friendly display names for all actors by [@​BagToad](https://github.com/BagToad) in [#​12854](cli/cli#12854) - `gh issue close`: add `--duplicate-of` flag and duplicate reason by [@​tksohishi](https://github.com/tksohishi) in [#​12811](cli/cli#12811) - `gh pr diff`: add `--exclude` flag to filter files from diff output by [@​yuvrajangadsingh](https://github.com/yuvrajangadsingh) in [#​12655](cli/cli#12655) - `gh pr view/list`: add `changeType` field to files JSON output by [@​yuvrajangadsingh](https://github.com/yuvrajangadsingh) in [#​12657](cli/cli#12657) - `gh repo clone`: add `--no-upstream` flag by [@​4RH1T3CT0R7](https://github.com/4RH1T3CT0R7) in [#​12686](cli/cli#12686) - `gh repo edit`: add `--squash-merge-commit-message` flag by [@​yuvrajangadsingh](https://github.com/yuvrajangadsingh) in [#​12846](cli/cli#12846) - `gh browse`: add `--blame` flag by [@​masonmcelvain](https://github.com/masonmcelvain) in [#​11486](cli/cli#11486) - `gh agent-task list`: add `--json` support by [@​maxbeizer](https://github.com/maxbeizer) in [#​12806](cli/cli#12806) - `gh agent-task view`: add `--json` support by [@​maxbeizer](https://github.com/maxbeizer) in [#​12807](cli/cli#12807) - `gh copilot`: set `COPILOT_GH` env var when launching Copilot CLI by [@​devm33](https://github.com/devm33) in [#​12821](cli/cli#12821) ##### 🐛 Fixes - Fix `gh project item-edit` error when editing Draft Issue with only one (`--title`/`--body`) flag by [@​ManManavadaria](https://github.com/ManManavadaria) in [#​12787](cli/cli#12787) - Fix extension install error message showing raw struct instead of `owner/repo` by [@​Copilot](https://github.com/Copilot) in [#​12836](cli/cli#12836) - Fix incorrect integer conversion from int to uint16 in port forwarder by [@​BagToad](https://github.com/BagToad) in [#​12831](cli/cli#12831) - Fix invalid ANSI SGR escape code in JSON and diff colorization by [@​BagToad](https://github.com/BagToad) in [#​12720](cli/cli#12720) - Fix assignees `databaseId` always being `0` in `--json` output by [@​srt32](https://github.com/srt32) in [#​12783](cli/cli#12783) - Fix error when `--remote` flag used with repo argument by [@​majiayu000](https://github.com/majiayu000) in [#​12375](cli/cli#12375) - Fix redundant API call in `gh issue view --comments` by [@​VishnuVV27](https://github.com/VishnuVV27) in [#​12652](cli/cli#12652) - Clarify scope error while creating issues for projects by [@​elijahthis](https://github.com/elijahthis) in [#​12596](cli/cli#12596) - Reject pull request-only search qualifiers in `gh issue list` by [@​LouisLau-art](https://github.com/LouisLau-art) in [#​12623](cli/cli#12623) - Prevent `.git/config` corruption on repeated `issue develop --name` invocation by [@​gunadhya](https://github.com/gunadhya) in [#​12651](cli/cli#12651) - Use pre-compiled regexp for matching Content-Type by [@​itchyny](https://github.com/itchyny) in [#​12781](cli/cli#12781) - Isolate generated licenses per platform (os/arch) by [@​babakks](https://github.com/babakks) in [#​12774](cli/cli#12774) ##### 📚 Docs & Chores - Add examples to `gh issue close` help text by [@​BagToad](https://github.com/BagToad) in [#​12830](cli/cli#12830) - Customizable install `prefix` in Makefile by [@​scarf005](https://github.com/scarf005) in [#​11714](cli/cli#11714) - Deduplicate scope error handling between `api/client.go` and project queries by [@​yuvrajangadsingh](https://github.com/yuvrajangadsingh) in [#​12845](cli/cli#12845) - Remove unnecessary `StateReason` and `StateReasonDuplicate` feature detection by [@​BagToad](https://github.com/BagToad) in [#​12838](cli/cli#12838) - Update Go version requirement to 1.26+ by [@​BagToad](https://github.com/BagToad) in [#​12864](cli/cli#12864) - Add monthly pitch surfacing workflow by [@​tidy-dev](https://github.com/tidy-dev) in [#​12894](cli/cli#12894) #####
Dependencies - Bump Go from 1.25.7 to 1.26.1 by [@​BagToad](https://github.com/BagToad) in [#​12860](cli/cli#12860) - chore(deps): bump golang.org/x/sync from 0.19.0 to 0.20.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12886](cli/cli#12886) - chore(deps): bump google.golang.org/grpc from 1.79.1 to 1.79.2 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12851](cli/cli#12851) - chore(deps): bump github.com/docker/cli from 29.0.3+incompatible to 29.2.0+incompatible by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12842](cli/cli#12842) - chore(deps): bump google.golang.org/grpc from 1.78.0 to 1.79.1 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12759](cli/cli#12759) - chore(deps): bump goreleaser/goreleaser-action from 6.4.0 to 7.0.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12760](cli/cli#12760) - chore(deps): bump actions/upload-artifact from 6 to 7 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12797](cli/cli#12797) - chore(deps): bump actions/download-artifact from 7 to 8 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12796](cli/cli#12796) - chore(deps): bump actions/attest-build-provenance from 3.2.0 to 4.1.0 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12795](cli/cli#12795) - chore(deps): bump github.com/gabriel-vasile/mimetype from 1.4.11 to 1.4.13 by [@​dependabot](https://github.com/dependabot)\[bot] in [#​12615](cli/cli#12615) #### New Contributors - [@​srt32](https://github.com/srt32) made their first contribution in [#​12783](cli/cli#12783) - [@​itchyny](https://github.com/itchyny) made their first contribution in [#​12781](cli/cli#12781) - [@​VishnuVV27](https://github.com/VishnuVV27) made their first contribution in [#​12652](cli/cli#12652) - [@​elijahthis](https://github.com/elijahthis) made their first contribution in [#​12596](cli/cli#12596) - [@​ManManavadaria](https://github.com/ManManavadaria) made their first contribution in [#​12787](cli/cli#12787) - [@​maxbeizer](https://github.com/maxbeizer) made their first contribution in [#​12806](cli/cli#12806) - [@​LouisLau-art](https://github.com/LouisLau-art) made their first contribution in [#​12623](cli/cli#12623) - [@​4RH1T3CT0R7](https://github.com/4RH1T3CT0R7) made their first contribution in [#​12686](cli/cli#12686) - [@​yuvrajangadsingh](https://github.com/yuvrajangadsingh) made their first contribution in [#​12657](cli/cli#12657) - [@​masonmcelvain](https://github.com/masonmcelvain) made their first contribution in [#​11486](cli/cli#11486) - [@​scarf005](https://github.com/scarf005) made their first contribution in [#​11714](cli/cli#11714) - [@​tksohishi](https://github.com/tksohishi) made their first contribution in [#​12811](cli/cli#12811) - [@​tidy-dev](https://github.com/tidy-dev) made their first contribution in [#​12894](cli/cli#12894) **Full Changelog**: <cli/cli@v2.87.3...v2.88.0> </details> --- ### Configuration 📅 **Schedule**: Branch creation - At any time (no schedule defined), Automerge - At any time (no schedule defined). 🚦 **Automerge**: Disabled by config. Please merge this manually once you are satisfied. ♻ **Rebasing**: Whenever MR becomes conflicted, or you tick the rebase/retry checkbox. 🔕 **Ignore**: Close this MR and you won't be reminded about this update again. --- - [ ] <!-- rebase-check -->If you want to rebase/retry this MR, check this box --- This MR has been generated by [Renovate Bot](https://github.com/renovatebot/renovate). <!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiI0My42MS43IiwidXBkYXRlZEluVmVyIjoiNDMuNjQuMyIsInRhcmdldEJyYW5jaCI6Im1haW4iLCJsYWJlbHMiOlsiUmVub3ZhdGUgQm90IiwiYXV0b21hdGlvbjpib3QtYXV0aG9yZWQiLCJkZXBlbmRlbmN5LXR5cGU6Om1pbm9yIl19-->
This PR fixes these issues regarding licenses generation/embedding at release time:
We've observed transient failures like this caused by GoReleaser pre-build hooks running in parallel and therefore stepping on each other. To fix this we're now isolating generated licenses per platform (os/arch), under
internal/licenses/embed/$GOOS-$GOARCH. This also means, we should have per-platform//go:embeds to make sure we take the right licenses. The per-platform files are named asembed_$GOOS_$GOARCH.go. Go compiler automatically picks the right file (without the need for explicit//go:build $GOOS && GOARCHbuild constraints).Currently, if we run
govulncheckon a released binary we get:Note the
+dirtysuffix. This is due to the generated files making the working directory dirty. The fix is to git-ignore the generated files. However, since we cannot check-in empty directories, we need to check in (empty)PLACEHOLDERfiles, but ignore the rest.The
embed_default.gofile works as a fallback for Go compiler, in case it's building against a not-supported platform. Without this file, users on unsupported platforms will fail to buildghfrom source. To verify this works, you can, for example, build for linux/mips and it shouldn't fail:Further verification
You should now be able to run, e.g.
./script/licenses linux amd64or./script/licenses darwin arm64, and then rungo run ./cmd/gh licensesand see the right licenses being reported. Also,git statusshould not show any uncommitted changes.