Copilot commented on code in PR #3493:
URL: https://github.com/apache/iceberg-rust/pull/3493#discussion_r4331586847
##########
bindings/python/LICENSE:
##########
Review Comment:
Shipping a placeholder `bindings/python/LICENSE` in the repository can lead
to non-compliant or confusing artifacts when developers build sdists/wheels
outside of the release workflows (e.g., local builds, downstream packagers,
forks, or alternative CI). Consider committing a valid `LICENSE` file in-tree
(at minimum the Apache 2.0 text, optionally with a clearly marked section for
generated third-party licenses), and have the workflow append/replace only the
third-party portion. This keeps non-workflow builds correct and improves
reliability/compliance.
##########
.github/workflows/release_python.yml:
##########
@@ -125,6 +125,20 @@ jobs:
with:
version: ${{ steps.version.outputs.pep440 }}
+ - name: Install cargo-about
+ uses: taiki-e/install-action@4cef1412cce204788f482e778a0b9187f9626a29
# v2.87.21
+ with:
+ tool: [email protected]
+
+ - name: Generate LICENSE with third-party licenses
+ shell: bash
+ run: |
+ # bindings/python/LICENSE is a placeholder: replace it with the root
LICENSE plus bundled dependency licenses.
+ {
+ cat LICENSE
+ cargo about generate -c bindings/python/about.toml --locked --fail
--manifest-path bindings/python/Cargo.toml bindings/python/about.hbs
+ } > bindings/python/LICENSE
Review Comment:
This step truncates `bindings/python/LICENSE` before running `cargo about
generate`. If `cargo about` fails partway through (e.g., transient IO/cargo
resolution issue), the workspace will be left with a partial/empty LICENSE,
which can make debugging harder and could affect subsequent steps if the shell
behavior changes. Prefer writing to a temporary file and moving it into place
only on success (atomic replace), optionally adding an explicit
newline/separator between the root LICENSE and the generated section.
##########
bindings/python/about.hbs:
##########
@@ -0,0 +1,63 @@
+{{!--
+Licensed to the Apache Software Foundation (ASF) under one
+or more contributor license agreements. See the NOTICE file
+distributed with this work for additional information
+regarding copyright ownership. The ASF licenses this file
+to you under the Apache License, Version 2.0 (the
+"License"); you may not use this file except in compliance
+with the License. You may obtain a copy of the License at
+
+ http://www.apache.org/licenses/LICENSE-2.0
+
+Unless required by applicable law or agreed to in writing,
+software distributed under the License is distributed on an
+"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+KIND, either express or implied. See the License for the
+specific language governing permissions and limitations
+under the License.
+--}}
+
+--------------------------------------------------------------------------------
+
+This package bundles Rust dependencies. Their licenses follow.
+{{#each licenses}}
+
+--------------------------------------------------------------------------------
+
+{{{name}}}
+
+Used by:
+{{#each used_by}}
+ - {{crate.name}} {{crate.version}}{{#if crate.repository}}
({{crate.repository}}){{/if}}
+{{/each}}
+
+{{{text}}}
+{{/each}}
+
+--------------------------------------------------------------------------------
+
+MIT License
+
+Used by:
+ - brotli-decompressor 5.0.3
(https://github.com/dropbox/rust-brotli-decompressor)
+ (src/context.rs)
Review Comment:
This section hardcodes a specific crate name/version and file path outside
the `{{#each licenses}}` loop, so it will become stale when the dependency
version changes or is removed. If this is an exception needed due to how
`cargo-about` renders license/notice data, prefer encoding it via `about.toml`
(e.g., a `clarify` entry keyed to the crate with checksums), or clearly
gate/structure the template so the extra block is tied to the actual dependency
metadata rather than a fixed version string.
##########
.github/workflows/release_python_nightly.yml:
##########
@@ -61,6 +61,20 @@ jobs:
with:
version: ${{ steps.version.outputs.pep440 }}
+ - name: Install cargo-about
+ uses: taiki-e/install-action@4cef1412cce204788f482e778a0b9187f9626a29
# v2.87.21
+ with:
+ tool: [email protected]
+
+ - name: Generate LICENSE with third-party licenses
+ shell: bash
+ run: |
+ # bindings/python/LICENSE is a placeholder: replace it with the root
LICENSE plus bundled dependency licenses.
+ {
+ cat LICENSE
+ cargo about generate -c bindings/python/about.toml --locked --fail
--manifest-path bindings/python/Cargo.toml bindings/python/about.hbs
+ } > bindings/python/LICENSE
Review Comment:
The `Install cargo-about` + `Generate LICENSE...` steps are duplicated
across multiple jobs and workflows (`release_python.yml` and
`release_python_nightly.yml`). Consider factoring this into a composite action
(e.g., `.github/actions/generate-third-party-licenses`) or a reusable workflow
step to reduce drift and ensure future updates (tool version, args, paths) are
applied consistently.
##########
bindings/python/NOTICE:
##########
Review Comment:
The NOTICE header says \"Apache Iceberg Rust\" but this file lives under
`bindings/python/` and is presumably intended for the `pyiceberg-core` Python
distribution. Consider aligning the product name with the Python
package/distribution name (or a more precise label like \"Apache Iceberg
(Python bindings)\") to avoid confusion for wheel consumers reading `dist-info`
notices.
--
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]
