chore(ci): Fail CI when a publishable crate is missing LICENSE or NOTICE - #3084
chore(ci): Fail CI when a publishable crate is missing LICENSE or NOTICE#3084dannycjones wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Adds a CI-enforced check ensuring all publishable workspace crates include LICENSE and NOTICE in their packaged artifacts, closing a gap where crates could be published missing required release files.
Changes:
- Introduces
dev/check_license_notice.shto validateLICENSE/NOTICEpresence incargo package --listoutput and verify the paths resolve on disk. - Adds a
make check-license-noticetarget and wires it intomake check. - Updates CI to run the new check and to no longer exclude
LICENSE/NOTICEchanges from PR-triggered CI.
Reviewed changes
Copilot reviewed 3 out of 3 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
dev/check_license_notice.sh |
New script to validate packaged artifacts contain LICENSE and NOTICE and that they resolve on disk. |
Makefile |
Adds a dedicated target for the new script and includes it in the aggregate check target. |
.github/workflows/ci.yml |
Ensures CI runs when LICENSE/NOTICE change and adds a step to run the new check. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Nice, the check works. https://github.com/apache/iceberg-rust/actions/runs/33081511431/job/98549689753?pr=3084 Now I'll address the violations. |
348df13 to
4d780e2
Compare
Missing LICENSE and NOTICE files for newly introduced crates (apache#2823) were only discovered manually during release preparation, because nothing asserted their presence. Add dev/check_license_notice.sh, which lists the packaged files of every publishable crate with `cargo package --list` and fails when a top-level LICENSE or NOTICE is absent. Checking the packaged file list rather than the crate directory also catches files present on disk but excluded from the published crate. Because `cargo package --list` reports an entry even when a symlinked target is gone, the file contents are checked separately so a removed or emptied root copy fails too. Each crate's copy must also resolve to the repository root file itself, so the license text cannot drift from one crate to the next. Run it from the CI lint job so pull requests fail when a crate is missing either file, and stop excluding the root LICENSE and NOTICE from the pull_request path filter, since every crate symlinks to those copies and a PR removing them must still run the job. Also wire it into `make check` so it can be run locally the same way as the other lint checks. The check does not pass at this commit. It reports iceberg-property-macro, which is published but ships neither file, and iceberg-catalog-s3tables, whose LICENSE and NOTICE are real copies rather than the repository root files. Both gaps are fixed in the two commits that follow. Closes apache#2827
iceberg-property-macro is published to crates.io but shipped neither a LICENSE nor a NOTICE, so the crate was distributed without the files an Apache release requires. It was missed because the crate was added after apache#1601, which introduced the files for the crates publishable at the time. Add the same relative symlinks to the repository root copies that every other publishable crate uses, which is what the new check in the preceding commit reports.
Every publishable crate references the root LICENSE and NOTICE through a relative symlink. That convention was established in apache#1601, which added the files to the crates that were publishable at the time. `iceberg-catalog-s3tables` was `publish = false` back then so it was skipped, and apache#1916 later added real copies when the crate was made publishable. Every crate added since has followed the symlink convention, leaving s3tables as the only outlier. Replace the copies with symlinks so the root files remain the single source of truth and per-crate copies cannot drift from them. This is also required for the check added earlier in this branch to pass, which rejects a per-crate copy that is not the root file itself: the s3tables copies are byte-identical to the root files today, but nothing kept them that way. `cargo package` dereferences the symlinks, so the published crate is unchanged: LICENSE and NOTICE are still regular files with byte-identical content.
4d780e2 to
236dbbf
Compare
|
|
||
| while IFS=$'\t' read -r crate manifest_path; do | ||
| check_crate "${crate}" "$(dirname "${manifest_path}")" || FAILED=1 | ||
| done < <(publishable_crates) |
There was a problem hiding this comment.
When we do < <(publishable_crates), the function in a subshell, and the parent never inspects that subshell's exit code. So if the publishable_crates function fails (e.g. a bad Cargo.toml), this loop won't run and it will show up as a CI false green.
To fix this, we just need to capture it into a variable first before going into the loop. When we do that the function runs in the process that runs the main script.
| while IFS=$'\t' read -r crate manifest_path; do | ||
| check_crate "${crate}" "$(dirname "${manifest_path}")" || FAILED=1 | ||
| done < <(publishable_crates) |
There was a problem hiding this comment.
| while IFS=$'\t' read -r crate manifest_path; do | |
| check_crate "${crate}" "$(dirname "${manifest_path}")" || FAILED=1 | |
| done < <(publishable_crates) | |
| crates="$(publishable_crates)" | |
| if [ -z "${crates}" ]; then | |
| report_error "No publishable crates found; cargo metadata may have failed." | |
| exit 1 | |
| fi | |
| while IFS=$'\t' read -r crate manifest_path; do | |
| check_crate "${crate}" "$(dirname "${manifest_path}")" || FAILED=1 | |
| done <<<"${crates}" |
blackmwk
left a comment
blackmwk
left a comment
There was a problem hiding this comment.
The overall approach looks sound, but the false-green failure path in the new check needs to be fixed before merging.
Please address the unresolved review thread on dev/check_license_notice.sh:168: failures from publishable_crates are not propagated through the process substitution, so the loop can process no crates and the script can still succeed. Capturing and validating the command output before entering the loop would preserve the failure status and ensure that an empty crate list cannot silently pass.
I reproduced this behavior under set -Eeuo pipefail: the parent continued after the process-substitution producer returned a nonzero status.
This review was drafted by an AI-assisted tool and
confirmed by an Apache Iceberg Rust maintainer. After you've
addressed the points above and pushed an update, an Apache Iceberg Rust
maintainer — a real person — will take the next look
at the PR. The findings cite the project's review criteria;
if you think one of them is mis-applied, please reply on the
PR and a maintainer will weigh in.More on how Apache Iceberg Rust handles maintainer review:
CONTRIBUTING.md.
Which issue does this PR close?
What changes are included in this PR?
This PR closes the gap where some crates were allowed to be published without LICENSE and NOTICE files. It does this by introducing a script that verifies the expected packaged artifact list and that the paths resolve to a real file
The checks are quite strict: they verify that the path is in the list, that it resolves to something non-empty, and that it resolves to the file in the root of the repository.
Are these changes tested?
Manual testing only.
AI Disclosure
Generative AI was used to author and test this change, with review and amendments before publishing the pull request.