You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@iceberg.apache.org by "dannycjones (via GitHub)" <gi...@apache.org> on 2026/10/12 16:34:12 UTC
[PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
dannycjones opened a new pull request, #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414
## Which issue does this PR close?
Kind of related to #3239.
## What changes are included in this PR?
Introduce clarifications for `zstd-sys` and `brotli-decompressor`. This doesn't impact the actual result - we're still meeting our allowlist of licenses. However, this accurately represents the dependencies we have.
Due to having no way to fail when clarifications go stale or fail to apply, the Python script checks diagnostic output to fail builds in these cases.
## Are these changes tested?
N/A
## AI Disclosure
Used LLM to help generate these scripts - especially how to calculate the hash for clarifications. I have reviewed, modified, and stand by the changes.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6270177265
FWIW, the complexity here comes because of two reasons:
- Clarifications come where the expression for the crate's distributed source code is not representative.
- `cargo-deny` failing open rather than closed, so we have to wrap it with extra checks.
I have opened issues for the former (here's the one for zstd-sys: https://github.com/gyscos/zstd-rs/issues/392). We could also take a look at the latter - that's ultimately where the scripts complexity comes from. I don't currently have spare bandwidth for that right now though.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "Copilot (via GitHub)" <gi...@apache.org>.
Copilot commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4250419629
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
Review Comment:
`declared_clarifications()` indexes `block[\"crate\"]` directly. If `deny.toml` has a malformed `[[licenses.clarify]]` entry (missing `crate` or non-string), this will raise `KeyError`/`TypeError` instead of producing a clear `Failure` with actionable context. Consider validating each block’s shape (and emitting a `Failure` that points to the offending clarify entry) before using the value.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
Review Comment:
`declared_clarifications()` indexes `block[\"crate\"]` directly. If `deny.toml` has a malformed `[[licenses.clarify]]` entry (missing `crate` or non-string), this will raise `KeyError`/`TypeError` instead of producing a clear `Failure` with actionable context. Consider validating each block’s shape (and emitting a `Failure` that points to the offending clarify entry) before using the value.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def evaluated_crates() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """Every crate cargo-deny evaluated, along with the subset that had a clarification applied."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+ if result.returncode != 0:
+ print(diagnostics, file=sys.stderr, end="")
+ raise Failure(
+ "cargo-deny failed while re-running to verify that no license "
+ "clarifications are ignored."
+ )
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ # Anything not accepted should already be reported.
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ # An `accepted` verdict covers exactly one crate. Other checks attach several.
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
Review Comment:
This check relies on an exact match of a human-readable cargo-deny label message. If cargo-deny changes the wording (even without changing semantics), builds will start failing because clarifications will be treated as not applied. To make this more robust, prefer matching a stable structured indicator if available in the JSON payload; if not, consider making the match tolerant (e.g., substring match on a key phrase) and documenting the cargo-deny version/message source this is tied to.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
Review Comment:
`declared_clarifications()` indexes `block[\"crate\"]` directly. If `deny.toml` has a malformed `[[licenses.clarify]]` entry (missing `crate` or non-string), this will raise `KeyError`/`TypeError` instead of producing a clear `Failure` with actionable context. Consider validating each block’s shape (and emitting a `Failure` that points to the offending clarify entry) before using the value.
##########
dev/release/licenses.py:
##########
@@ -68,6 +351,12 @@ def command_check_dependencies(_args: argparse.Namespace) -> None:
if result.returncode != 0:
raise Failure("cargo-deny rejected the dependency licenses.")
+ declared = declared_clarifications()
+ evaluated, clarified = evaluated_crates()
+
+ require_clarifications_applied(declared, evaluated, clarified)
+ require_no_stale_clarifications(declared, evaluated)
Review Comment:
`command_check_dependencies` runs `cargo deny check license` and then always re-runs it again inside `evaluated_crates()`. This doubles cargo-deny runtime in CI. Consider running cargo-deny only once (e.g., run the JSON-formatted invocation and use its return code for the main failure condition), or conditionally skipping the second run when there are no `[[licenses.clarify]]` blocks to validate.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def evaluated_crates() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """Every crate cargo-deny evaluated, along with the subset that had a clarification applied."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+ if result.returncode != 0:
+ print(diagnostics, file=sys.stderr, end="")
+ raise Failure(
+ "cargo-deny failed while re-running to verify that no license "
+ "clarifications are ignored."
+ )
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ # Anything not accepted should already be reported.
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ # An `accepted` verdict covers exactly one crate. Other checks attach several.
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
+ clarified[name].add(version)
+
+ return dict(evaluated), dict(clarified)
+
+
+def cargo_metadata() -> dict:
+ """Resolved cargo metadata for this workspace."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "metadata",
+ "--format-version",
+ "1",
+ "--locked",
+ "--all-features",
+ "--manifest-path",
+ str(REPO_ROOT / "Cargo.toml"),
+ ],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ raise Failure(f"cargo metadata failed:\n{result.stderr}")
+ return json.loads(result.stdout)
+
+
+def crate_source_dirs(crates: set[Crate]) -> dict[Crate, str]:
+ """Where cargo unpacked each crate's sources.
+
+ The directory is `<name>-<version>`, but names often contain hyphens (eg. my-crate-5.0.3) and
+ versions can carry build metadata (eg. my-crate-5.0.3+mybuild).
+ """
+ packages = cargo_metadata()["packages"]
+
+ dict_crate_to_path = {}
+
+ for package in packages:
+ crate = (package["name"], package["version"])
+ if crate not in crates:
+ continue
+ dict_crate_to_path[crate] = str(Path(package["manifest_path"]).parent)
+
+ return dict_crate_to_path
+
+
+def require_clarifications_applied(
+ declared: set[CrateName],
+ evaluated: dict[CrateName, set[CrateVersion]],
+ actually_clarified: dict[CrateName, set[CrateVersion]],
+) -> None:
+ """
+ Fail if a clarification did not reach a crate version that was evaluated.
+
+ `declared` is all crates declared with a clarification in deny.toml,
+ `evaluated` is the list of all crate versions evaluated as part of the dependency graph,
+ and `actually_clarified` is the subset of crate versions evaluated which had a clarification applied.
+ """
+ crates_with_clarifications_missed: list[Crate] = []
+ for crate_name in declared:
+ if crate_name not in evaluated:
+ continue
+ versions_evaluated = evaluated[crate_name]
+ versions_clarified = actually_clarified.get(crate_name, set())
+ for crate_version in versions_evaluated - versions_clarified:
+ crates_with_clarifications_missed.append((crate_name, crate_version))
+ crates_with_clarifications_missed.sort()
+ if not crates_with_clarifications_missed:
+ # Nothing to report
+ return
+
+ source_dirs = crate_source_dirs(set(crates_with_clarifications_missed))
+ error_line_output = []
+ for crate_name, crate_version in crates_with_clarifications_missed:
+ source_dir = source_dirs.get(
+ (crate_name, crate_version), "<source not unpacked>"
+ )
+ error_line_output.append(
+ f" {crate_name} {crate_version} ({source_dir}): clarification did not apply"
+ )
+
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} declares license clarifications that cargo-deny did not apply:
+ {crates}
+
+ A clarification is discarded by cargo-deny when any of its attested license
+ files are no longer valid (hash mismatch, etc.).
+
+ Review the clarification, checking all files still exist and the hash matches for each.
+ You can compute the hash for a license file using the following command:
+
+ uv run {script} dependencies clarification-hash \\
+ <crate directory above>/<license file path>
+
+ If a hash has changed, read the file and ensure the license expression still matches.
+ If it does, an updated deny.toml should be committed.
+ """).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(error_line_output),
+ script=SCRIPT_PATH,
+ )
+ )
+
+
+def require_no_stale_clarifications(
+ declared: set[CrateName], evaluated: dict[CrateName, set[CrateVersion]]
+) -> None:
+ """Fail if a clarification names a crate that is not in the dependency graph."""
+ stale: list[CrateName] = sorted(declared - evaluated.keys())
+ if not stale:
+ return
+
+ raise Failure(
+ textwrap.dedent(
+ """
+ {deny_toml} declares license clarifications for crates that cargo-deny did
+ not find in the dependency graph:
+ {crates}
+
+ These are likely stale entries that should be removed.
+ """
+ ).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(f" {crate_name}" for crate_name in stale),
+ )
+ )
+
+
+def clarification_file_hash(path: Path) -> str:
+ """The hash cargo-deny expects in a [[licenses.clarify]] `license-files` entry.
+
+ XxHash32 with seed 0, over the file with every line terminated by exactly one LF.
+
+ cargo-deny appears to offer no way to compute this value,
+ so this command is added to make it easy (based on the cargo-deny source code).
+ """
+ try:
+ # Import here, so other commands of the script can still work if missing.
+ import xxhash
+ except ModuleNotFoundError:
+ raise Failure(
+ "The module 'xxhash' is required to compute the hash for cargo-deny's deny.toml file."
+ )
+
+ lines = path.read_bytes().split(b"\n")
+ # Splitting on \n may leave final element with only line feed - drop it.
+ if lines and lines[-1] == b"":
+ lines.pop()
+ # Drop any line feed, ensure every line ends '\n'
+ text = b"".join(line.rstrip(b"\r") + b"\n" for line in lines)
+
+ digest = xxhash.xxh32(text, seed=0).intdigest()
+ return f"0x{digest:08x}"
+
+
+def command_clarification_hash(args: argparse.Namespace) -> None:
+ """Print the clarification hash of one license file, and nothing else."""
+ path: Path = args.file
+ if not path.is_file():
+ raise Failure(f"No such file: {path}")
+ print(clarification_file_hash(path))
+
+
def command_check_dependencies(_args: argparse.Namespace) -> None:
step("Check dependency licenses")
require_cargo_deny()
Review Comment:
`command_check_dependencies` runs `cargo deny check license` and then always re-runs it again inside `evaluated_crates()`. This doubles cargo-deny runtime in CI. Consider running cargo-deny only once (e.g., run the JSON-formatted invocation and use its return code for the main failure condition), or conditionally skipping the second run when there are no `[[licenses.clarify]]` blocks to validate.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def evaluated_crates() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """Every crate cargo-deny evaluated, along with the subset that had a clarification applied."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+ if result.returncode != 0:
+ print(diagnostics, file=sys.stderr, end="")
+ raise Failure(
+ "cargo-deny failed while re-running to verify that no license "
+ "clarifications are ignored."
+ )
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ # Anything not accepted should already be reported.
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ # An `accepted` verdict covers exactly one crate. Other checks attach several.
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
+ clarified[name].add(version)
+
+ return dict(evaluated), dict(clarified)
+
+
+def cargo_metadata() -> dict:
+ """Resolved cargo metadata for this workspace."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "metadata",
+ "--format-version",
+ "1",
+ "--locked",
+ "--all-features",
+ "--manifest-path",
+ str(REPO_ROOT / "Cargo.toml"),
+ ],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ raise Failure(f"cargo metadata failed:\n{result.stderr}")
+ return json.loads(result.stdout)
+
+
+def crate_source_dirs(crates: set[Crate]) -> dict[Crate, str]:
+ """Where cargo unpacked each crate's sources.
+
+ The directory is `<name>-<version>`, but names often contain hyphens (eg. my-crate-5.0.3) and
+ versions can carry build metadata (eg. my-crate-5.0.3+mybuild).
+ """
+ packages = cargo_metadata()["packages"]
+
+ dict_crate_to_path = {}
+
+ for package in packages:
+ crate = (package["name"], package["version"])
+ if crate not in crates:
+ continue
+ dict_crate_to_path[crate] = str(Path(package["manifest_path"]).parent)
+
+ return dict_crate_to_path
+
+
+def require_clarifications_applied(
+ declared: set[CrateName],
+ evaluated: dict[CrateName, set[CrateVersion]],
+ actually_clarified: dict[CrateName, set[CrateVersion]],
+) -> None:
+ """
+ Fail if a clarification did not reach a crate version that was evaluated.
+
+ `declared` is all crates declared with a clarification in deny.toml,
+ `evaluated` is the list of all crate versions evaluated as part of the dependency graph,
+ and `actually_clarified` is the subset of crate versions evaluated which had a clarification applied.
+ """
+ crates_with_clarifications_missed: list[Crate] = []
+ for crate_name in declared:
+ if crate_name not in evaluated:
+ continue
+ versions_evaluated = evaluated[crate_name]
+ versions_clarified = actually_clarified.get(crate_name, set())
+ for crate_version in versions_evaluated - versions_clarified:
+ crates_with_clarifications_missed.append((crate_name, crate_version))
+ crates_with_clarifications_missed.sort()
+ if not crates_with_clarifications_missed:
+ # Nothing to report
+ return
+
+ source_dirs = crate_source_dirs(set(crates_with_clarifications_missed))
+ error_line_output = []
+ for crate_name, crate_version in crates_with_clarifications_missed:
+ source_dir = source_dirs.get(
+ (crate_name, crate_version), "<source not unpacked>"
+ )
+ error_line_output.append(
+ f" {crate_name} {crate_version} ({source_dir}): clarification did not apply"
+ )
+
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} declares license clarifications that cargo-deny did not apply:
+ {crates}
+
+ A clarification is discarded by cargo-deny when any of its attested license
+ files are no longer valid (hash mismatch, etc.).
+
+ Review the clarification, checking all files still exist and the hash matches for each.
+ You can compute the hash for a license file using the following command:
+
+ uv run {script} dependencies clarification-hash \\
+ <crate directory above>/<license file path>
+
+ If a hash has changed, read the file and ensure the license expression still matches.
+ If it does, an updated deny.toml should be committed.
+ """).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(error_line_output),
+ script=SCRIPT_PATH,
+ )
+ )
+
+
+def require_no_stale_clarifications(
+ declared: set[CrateName], evaluated: dict[CrateName, set[CrateVersion]]
+) -> None:
+ """Fail if a clarification names a crate that is not in the dependency graph."""
+ stale: list[CrateName] = sorted(declared - evaluated.keys())
+ if not stale:
+ return
+
+ raise Failure(
+ textwrap.dedent(
+ """
+ {deny_toml} declares license clarifications for crates that cargo-deny did
+ not find in the dependency graph:
+ {crates}
+
+ These are likely stale entries that should be removed.
+ """
+ ).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(f" {crate_name}" for crate_name in stale),
+ )
+ )
+
+
+def clarification_file_hash(path: Path) -> str:
+ """The hash cargo-deny expects in a [[licenses.clarify]] `license-files` entry.
+
+ XxHash32 with seed 0, over the file with every line terminated by exactly one LF.
+
+ cargo-deny appears to offer no way to compute this value,
+ so this command is added to make it easy (based on the cargo-deny source code).
+ """
+ try:
+ # Import here, so other commands of the script can still work if missing.
+ import xxhash
+ except ModuleNotFoundError:
+ raise Failure(
+ "The module 'xxhash' is required to compute the hash for cargo-deny's deny.toml file."
+ )
+
+ lines = path.read_bytes().split(b"\n")
+ # Splitting on \n may leave final element with only line feed - drop it.
+ if lines and lines[-1] == b"":
+ lines.pop()
+ # Drop any line feed, ensure every line ends '\n'
+ text = b"".join(line.rstrip(b"\r") + b"\n" for line in lines)
Review Comment:
Line 324 appears to contain a non-ASCII whitespace character after `#` (likely a non-breaking space), which can be confusing in reviews and can cause subtle linting/formatting issues. Also, the comment wording is a bit inaccurate (the trailing element is empty due to a trailing newline, not “only line feed”). Replacing the NBSP with a normal space and tightening the comment would make this clearer.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4293433305
##########
dev/release/licenses.py:
##########
@@ -56,17 +75,296 @@ def require_cargo_deny() -> None:
)
-def command_check_dependencies(_args: argparse.Namespace) -> None:
- step("Check dependency licenses")
- require_cargo_deny()
+def parse_declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def run_cargo_deny() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """
+ Run cargo-deny, returning mappings from crate name to crate version
+ for every crate version evaluated plus the subset which had a clarification applied.
+ """
result = subprocess.run(
- ["cargo", "deny", "check", "license"],
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ errors: list[str] = []
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ if fields.get("severity") == "error" or fields.get("level") == "ERROR":
+ krate = (fields.get("graphs") or [{}])[0].get("Krate", {})
+ crate = f"{krate['name']} {krate['version']}: " if krate else ""
+ labels = fields.get("labels") or []
+
+ error_msg = f" {crate}{fields.get('message')}"
+ if fields.get("code") == "rejected":
+ # For rejections, include the expression. Other codes are too noisy (such as TOML error).
+ span = labels[0].get("span")
+ error_msg = f"{error_msg} ({span})"
+ errors.append(error_msg)
+
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+
+ # There's no machine readable error code.
+ # If this error message changes, the script will fail closed anyway as expected clarifications won't be found.
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
+ clarified[name].add(version)
+
+ if result.returncode != 0:
+ # Emit list of errors, or simply diagnostics if error code is non-zero but we failed to parse any errors.
+ detail = "\n".join(errors) if errors else diagnostics.rstrip("\n")
+ raise Failure(f"cargo-deny rejected the dependency licenses:\n{detail}")
+
+ return dict(evaluated), dict(clarified)
+
+
+def cargo_metadata() -> dict:
+ """Resolved cargo metadata for this workspace."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "metadata",
+ "--format-version",
+ "1",
+ "--locked",
+ "--all-features",
+ "--manifest-path",
+ str(REPO_ROOT / "Cargo.toml"),
+ ],
+ capture_output=True,
+ text=True,
check=False,
)
if result.returncode != 0:
- raise Failure("cargo-deny rejected the dependency licenses.")
+ raise Failure(f"cargo metadata failed:\n{result.stderr}")
+ return json.loads(result.stdout)
+
+
+def crate_source_dirs(crates: set[Crate]) -> dict[Crate, str]:
+ """Where cargo unpacked each crate's sources.
+
+ The directory is `<name>-<version>`, but names often contain hyphens (eg. my-crate-5.0.3) and
+ versions can carry build metadata (eg. my-crate-5.0.3+mybuild).
+ """
+ packages = cargo_metadata()["packages"]
+
+ dict_crate_to_path = {}
+
+ for package in packages:
+ crate = (package["name"], package["version"])
+ if crate not in crates:
+ continue
+ dict_crate_to_path[crate] = str(Path(package["manifest_path"]).parent)
+
+ return dict_crate_to_path
+
+
+def require_clarifications_applied(
+ declared: set[CrateName],
+ evaluated: dict[CrateName, set[CrateVersion]],
+ actually_clarified: dict[CrateName, set[CrateVersion]],
+) -> None:
+ """
+ Fail if a clarification did not reach a crate version that was evaluated.
+
+ `declared` is all crates declared with a clarification in deny.toml,
+ `evaluated` is the list of all crate versions evaluated as part of the dependency graph,
+ and `actually_clarified` is the subset of crate versions evaluated which had a clarification applied.
+ """
+ crates_with_clarifications_missed: list[Crate] = []
+ for crate_name in declared:
+ if crate_name not in evaluated:
+ continue
+ versions_evaluated = evaluated[crate_name]
+ versions_clarified = actually_clarified.get(crate_name, set())
+ for crate_version in versions_evaluated - versions_clarified:
+ crates_with_clarifications_missed.append((crate_name, crate_version))
+ crates_with_clarifications_missed.sort()
+ if not crates_with_clarifications_missed:
+ # Nothing to report
+ return
+
+ source_dirs = crate_source_dirs(set(crates_with_clarifications_missed))
+ error_line_output = []
+ for crate_name, crate_version in crates_with_clarifications_missed:
+ source_dir = source_dirs.get(
+ (crate_name, crate_version), "<source not unpacked>"
+ )
+ error_line_output.append(
+ f" {crate_name} {crate_version} ({source_dir}): clarification did not apply"
+ )
+
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} declares license clarifications that cargo-deny did not apply:
+ {crates}
+
+ A clarification is discarded by cargo-deny when any of its attested license
+ files are no longer valid (hash mismatch, etc.).
+
+ Review the clarification, checking all files still exist and the hash matches for each.
+ You can compute the hash for a license file using the following command:
+
+ uv run {script} dependencies clarification-hash \\
+ <crate directory above>/<license file path>
+
+ If a hash has changed, read the file and ensure the license expression still matches.
+ If it does, an updated deny.toml should be committed.
+ """).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(error_line_output),
+ script=SCRIPT_PATH,
+ )
+ )
+
+
+def require_no_stale_clarifications(
+ declared: set[CrateName], evaluated: dict[CrateName, set[CrateVersion]]
+) -> None:
+ """Fail if a clarification names a crate that is not in the dependency graph."""
+ stale: list[CrateName] = sorted(declared - evaluated.keys())
+ if not stale:
+ return
+
+ raise Failure(
+ textwrap.dedent(
+ """
+ {deny_toml} declares license clarifications for crates that cargo-deny did
+ not find in the dependency graph:
+ {crates}
+
+ These are likely stale entries that should be removed.
+ """
+ ).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(f" {crate_name}" for crate_name in stale),
+ )
+ )
+
+
+def clarification_file_hash(path: Path) -> str:
+ """The hash cargo-deny expects in a [[licenses.clarify]] `license-files` entry.
+
+ XxHash32 with seed 0, over the file with every line terminated by exactly one LF.
+
+ cargo-deny appears to offer no way to compute this value,
+ so this command is added to make it easy (based on the cargo-deny source code).
Review Comment:
I'll reword to clarify its not copied.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4285525256
##########
dev/release/licenses.py:
##########
@@ -68,6 +351,12 @@ def command_check_dependencies(_args: argparse.Namespace) -> None:
if result.returncode != 0:
raise Failure("cargo-deny rejected the dependency licenses.")
+ declared = declared_clarifications()
+ evaluated, clarified = evaluated_crates()
+
+ require_clarifications_applied(declared, evaluated, clarified)
+ require_no_stale_clarifications(declared, evaluated)
Review Comment:
OK I'm just going to run it once, I think its clearer logically even if we lose the better human-readable output.
Ideally, this is failing very very rarely...
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6265548582
> do we really need these clarifications? im worried about the maintenance burden of adding this PR.
Ultimately, it's about correctness of the license declared by the crates. It doesn't materially impact the outcome - all the licenses are accepted by our policy anyway.
We can skip it and it would be fine, but then I'm not sure what we could say about that (and LLM will flag it again and again).
I do agree that its growing the complexity here (and ideally it would live upstream in `cargo-deny`). I'm hoping its set and forget, but I'm happy to remove it if you feel strongly on this.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260777147
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def evaluated_crates() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """Every crate cargo-deny evaluated, along with the subset that had a clarification applied."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+ if result.returncode != 0:
+ print(diagnostics, file=sys.stderr, end="")
+ raise Failure(
+ "cargo-deny failed while re-running to verify that no license "
+ "clarifications are ignored."
+ )
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ # Anything not accepted should already be reported.
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ # An `accepted` verdict covers exactly one crate. Other checks attach several.
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
+ clarified[name].add(version)
+
+ return dict(evaluated), dict(clarified)
+
+
+def cargo_metadata() -> dict:
+ """Resolved cargo metadata for this workspace."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "metadata",
+ "--format-version",
+ "1",
+ "--locked",
+ "--all-features",
+ "--manifest-path",
+ str(REPO_ROOT / "Cargo.toml"),
+ ],
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ raise Failure(f"cargo metadata failed:\n{result.stderr}")
+ return json.loads(result.stdout)
+
+
+def crate_source_dirs(crates: set[Crate]) -> dict[Crate, str]:
+ """Where cargo unpacked each crate's sources.
+
+ The directory is `<name>-<version>`, but names often contain hyphens (eg. my-crate-5.0.3) and
+ versions can carry build metadata (eg. my-crate-5.0.3+mybuild).
+ """
+ packages = cargo_metadata()["packages"]
+
+ dict_crate_to_path = {}
+
+ for package in packages:
+ crate = (package["name"], package["version"])
+ if crate not in crates:
+ continue
+ dict_crate_to_path[crate] = str(Path(package["manifest_path"]).parent)
+
+ return dict_crate_to_path
+
+
+def require_clarifications_applied(
+ declared: set[CrateName],
+ evaluated: dict[CrateName, set[CrateVersion]],
+ actually_clarified: dict[CrateName, set[CrateVersion]],
+) -> None:
+ """
+ Fail if a clarification did not reach a crate version that was evaluated.
+
+ `declared` is all crates declared with a clarification in deny.toml,
+ `evaluated` is the list of all crate versions evaluated as part of the dependency graph,
+ and `actually_clarified` is the subset of crate versions evaluated which had a clarification applied.
+ """
+ crates_with_clarifications_missed: list[Crate] = []
+ for crate_name in declared:
+ if crate_name not in evaluated:
+ continue
+ versions_evaluated = evaluated[crate_name]
+ versions_clarified = actually_clarified.get(crate_name, set())
+ for crate_version in versions_evaluated - versions_clarified:
+ crates_with_clarifications_missed.append((crate_name, crate_version))
+ crates_with_clarifications_missed.sort()
+ if not crates_with_clarifications_missed:
+ # Nothing to report
+ return
+
+ source_dirs = crate_source_dirs(set(crates_with_clarifications_missed))
+ error_line_output = []
+ for crate_name, crate_version in crates_with_clarifications_missed:
+ source_dir = source_dirs.get(
+ (crate_name, crate_version), "<source not unpacked>"
+ )
+ error_line_output.append(
+ f" {crate_name} {crate_version} ({source_dir}): clarification did not apply"
+ )
+
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} declares license clarifications that cargo-deny did not apply:
+ {crates}
+
+ A clarification is discarded by cargo-deny when any of its attested license
+ files are no longer valid (hash mismatch, etc.).
+
+ Review the clarification, checking all files still exist and the hash matches for each.
+ You can compute the hash for a license file using the following command:
+
+ uv run {script} dependencies clarification-hash \\
+ <crate directory above>/<license file path>
+
+ If a hash has changed, read the file and ensure the license expression still matches.
+ If it does, an updated deny.toml should be committed.
+ """).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(error_line_output),
+ script=SCRIPT_PATH,
+ )
+ )
+
+
+def require_no_stale_clarifications(
+ declared: set[CrateName], evaluated: dict[CrateName, set[CrateVersion]]
+) -> None:
+ """Fail if a clarification names a crate that is not in the dependency graph."""
+ stale: list[CrateName] = sorted(declared - evaluated.keys())
+ if not stale:
+ return
+
+ raise Failure(
+ textwrap.dedent(
+ """
+ {deny_toml} declares license clarifications for crates that cargo-deny did
+ not find in the dependency graph:
+ {crates}
+
+ These are likely stale entries that should be removed.
+ """
+ ).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(f" {crate_name}" for crate_name in stale),
+ )
+ )
+
+
+def clarification_file_hash(path: Path) -> str:
+ """The hash cargo-deny expects in a [[licenses.clarify]] `license-files` entry.
+
+ XxHash32 with seed 0, over the file with every line terminated by exactly one LF.
+
+ cargo-deny appears to offer no way to compute this value,
+ so this command is added to make it easy (based on the cargo-deny source code).
+ """
+ try:
+ # Import here, so other commands of the script can still work if missing.
+ import xxhash
+ except ModuleNotFoundError:
+ raise Failure(
+ "The module 'xxhash' is required to compute the hash for cargo-deny's deny.toml file."
+ )
+
+ lines = path.read_bytes().split(b"\n")
+ # Splitting on \n may leave final element with only line feed - drop it.
+ if lines and lines[-1] == b"":
+ lines.pop()
+ # Drop any line feed, ensure every line ends '\n'
+ text = b"".join(line.rstrip(b"\r") + b"\n" for line in lines)
+
+ digest = xxhash.xxh32(text, seed=0).intdigest()
+ return f"0x{digest:08x}"
+
+
+def command_clarification_hash(args: argparse.Namespace) -> None:
+ """Print the clarification hash of one license file, and nothing else."""
+ path: Path = args.file
+ if not path.is_file():
+ raise Failure(f"No such file: {path}")
+ print(clarification_file_hash(path))
+
+
def command_check_dependencies(_args: argparse.Namespace) -> None:
step("Check dependency licenses")
require_cargo_deny()
Review Comment:
OK, I may opt to make this change. I was originally okay with running it twice such that you see clear and familiar output when a normal error is reported, but I think it's weird, unclear, and wasteful to run twice.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260780066
##########
dev/release/licenses.py:
##########
@@ -68,6 +351,12 @@ def command_check_dependencies(_args: argparse.Namespace) -> None:
if result.returncode != 0:
raise Failure("cargo-deny rejected the dependency licenses.")
+ declared = declared_clarifications()
+ evaluated, clarified = evaluated_crates()
+
+ require_clarifications_applied(declared, evaluated, clarified)
+ require_no_stale_clarifications(declared, evaluated)
Review Comment:
https://github.com/apache/iceberg-rust/pull/3414#discussion_r4250419799
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260733893
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
Review Comment:
I actually had implemented checking here, but I removed it to keep the code simpler and easier to follow.
I'm happy to be challenged on this, and we can reintroduce the checks. Reviewers, please let me know if this is a concern.
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
Review Comment:
I actually had implemented checking here, but I removed it to keep the code simpler and easier to follow.
I'm happy to be challenged on this, and we can reintroduce the checks. Reviewers, please let me know if this is a concern.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6302466229
I'd like to keep the `dev/release/licenses.py` script as simple as possible. On `main`, it just runs cargo deny.
This PR's changes introduce a lot of complexity and maintenance that I'd like to avoid if possible.
I was able to address zstd by removing the legacy feature (#3495)
I also split out the `unused-allowed-license` and `unused-license-exception` changes, i think they are great additions (#3497)
I put up a simpler solution in #3493, please lmk what you think!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6315708793
> I share same concern with @kevinjqliu , this change is over complicated. If the clarification outdated, cargo deny will fail, and that's what exactly we want.
This isn't true though, it doesn't fail. Clarifications are just ignored if they don't match perfectly. That's what led me to build a wrapping script.
I'm OK with us accepting that the cost is too high and rely on the current state being OK. I do share the concern there on maintenance burden. The other approach I'm looking at is that we make sure these checks are handled upstream in `cargo-deny` and `cargo-about`, which would remove the burden on us and put the code in the right place and benefit all.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4313043780
##########
deny.toml:
##########
@@ -43,3 +47,33 @@ exceptions = [
{ allow = ["MPL-2.0"], crate = "colored" },
{ allow = ["MPL-2.0"], crate = "option-ext" },
]
+
+# Clarifications use a hash to verify if the license file bytes changed.
+# To recompute: uv run dev/release/licenses.py dependencies clarification-hash <file>
+
+# TODO: zstd-sys 2.1 relicensed the wrapper and bindings to BSD-3-Clause.
+# Transient dependency, drop when we upgrade (likely when we upgrade to Arrow 60).
+# https://github.com/apache/iceberg-rust/issues/3307
+[[licenses.clarify]]
+crate = "zstd-sys"
+expression = "(MIT OR Apache-2.0) AND BSD-3-Clause AND (GPL-2.0-only OR BSD-3-Clause)"
Review Comment:
It will be via `cargo-about` which has very similar syntax. For a license embedded in the file itself, you give it a line and char offset (and checksum) and it'll copy just the license bytes across.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "CTTY (via GitHub)" <gi...@apache.org>.
CTTY commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4300027121
##########
deny.toml:
##########
@@ -43,3 +47,33 @@ exceptions = [
{ allow = ["MPL-2.0"], crate = "colored" },
{ allow = ["MPL-2.0"], crate = "option-ext" },
]
+
+# Clarifications use a hash to verify if the license file bytes changed.
+# To recompute: uv run dev/release/licenses.py dependencies clarification-hash <file>
+
+# TODO: zstd-sys 2.1 relicensed the wrapper and bindings to BSD-3-Clause.
+# Transient dependency, drop when we upgrade (likely when we upgrade to Arrow 60).
+# https://github.com/apache/iceberg-rust/issues/3307
+[[licenses.clarify]]
+crate = "zstd-sys"
+expression = "(MIT OR Apache-2.0) AND BSD-3-Clause AND (GPL-2.0-only OR BSD-3-Clause)"
Review Comment:
It sounds like we will run binary attribution verification in a different way other than the existing script?
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6295510339
i think we can be a bit more lenient with the asf policy interpretation because the python packages are convenient artifacts. let me push up something i have locally
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6315783944
I'll close this for now since all three of us are concerned about maintenance cost here.
I'll follow up at a later date to see what checks we can add upstream in `cargo-about`.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260735518
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
Review Comment:
https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260733893
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4292374922
##########
dev/release/licenses.py:
##########
@@ -56,17 +75,296 @@ def require_cargo_deny() -> None:
)
-def command_check_dependencies(_args: argparse.Namespace) -> None:
- step("Check dependency licenses")
- require_cargo_deny()
+def parse_declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def run_cargo_deny() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """
+ Run cargo-deny, returning mappings from crate name to crate version
+ for every crate version evaluated plus the subset which had a clarification applied.
+ """
result = subprocess.run(
- ["cargo", "deny", "check", "license"],
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ errors: list[str] = []
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ if fields.get("severity") == "error" or fields.get("level") == "ERROR":
+ krate = (fields.get("graphs") or [{}])[0].get("Krate", {})
+ crate = f"{krate['name']} {krate['version']}: " if krate else ""
+ labels = fields.get("labels") or []
+
+ error_msg = f" {crate}{fields.get('message')}"
+ if fields.get("code") == "rejected":
+ # For rejections, include the expression. Other codes are too noisy (such as TOML error).
+ span = labels[0].get("span")
+ error_msg = f"{error_msg} ({span})"
+ errors.append(error_msg)
+
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+
+ # There's no machine readable error code.
+ # If this error message changes, the script will fail closed anyway as expected clarifications won't be found.
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
+ clarified[name].add(version)
+
+ if result.returncode != 0:
+ # Emit list of errors, or simply diagnostics if error code is non-zero but we failed to parse any errors.
+ detail = "\n".join(errors) if errors else diagnostics.rstrip("\n")
+ raise Failure(f"cargo-deny rejected the dependency licenses:\n{detail}")
+
+ return dict(evaluated), dict(clarified)
+
+
+def cargo_metadata() -> dict:
+ """Resolved cargo metadata for this workspace."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "metadata",
+ "--format-version",
+ "1",
+ "--locked",
+ "--all-features",
+ "--manifest-path",
+ str(REPO_ROOT / "Cargo.toml"),
+ ],
+ capture_output=True,
+ text=True,
check=False,
)
if result.returncode != 0:
- raise Failure("cargo-deny rejected the dependency licenses.")
+ raise Failure(f"cargo metadata failed:\n{result.stderr}")
+ return json.loads(result.stdout)
+
+
+def crate_source_dirs(crates: set[Crate]) -> dict[Crate, str]:
+ """Where cargo unpacked each crate's sources.
+
+ The directory is `<name>-<version>`, but names often contain hyphens (eg. my-crate-5.0.3) and
+ versions can carry build metadata (eg. my-crate-5.0.3+mybuild).
+ """
+ packages = cargo_metadata()["packages"]
+
+ dict_crate_to_path = {}
+
+ for package in packages:
+ crate = (package["name"], package["version"])
+ if crate not in crates:
+ continue
+ dict_crate_to_path[crate] = str(Path(package["manifest_path"]).parent)
+
+ return dict_crate_to_path
+
+
+def require_clarifications_applied(
+ declared: set[CrateName],
+ evaluated: dict[CrateName, set[CrateVersion]],
+ actually_clarified: dict[CrateName, set[CrateVersion]],
+) -> None:
+ """
+ Fail if a clarification did not reach a crate version that was evaluated.
+
+ `declared` is all crates declared with a clarification in deny.toml,
+ `evaluated` is the list of all crate versions evaluated as part of the dependency graph,
+ and `actually_clarified` is the subset of crate versions evaluated which had a clarification applied.
+ """
+ crates_with_clarifications_missed: list[Crate] = []
+ for crate_name in declared:
+ if crate_name not in evaluated:
+ continue
+ versions_evaluated = evaluated[crate_name]
+ versions_clarified = actually_clarified.get(crate_name, set())
+ for crate_version in versions_evaluated - versions_clarified:
+ crates_with_clarifications_missed.append((crate_name, crate_version))
+ crates_with_clarifications_missed.sort()
+ if not crates_with_clarifications_missed:
+ # Nothing to report
+ return
+
+ source_dirs = crate_source_dirs(set(crates_with_clarifications_missed))
+ error_line_output = []
+ for crate_name, crate_version in crates_with_clarifications_missed:
+ source_dir = source_dirs.get(
+ (crate_name, crate_version), "<source not unpacked>"
+ )
+ error_line_output.append(
+ f" {crate_name} {crate_version} ({source_dir}): clarification did not apply"
+ )
+
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} declares license clarifications that cargo-deny did not apply:
+ {crates}
+
+ A clarification is discarded by cargo-deny when any of its attested license
+ files are no longer valid (hash mismatch, etc.).
+
+ Review the clarification, checking all files still exist and the hash matches for each.
+ You can compute the hash for a license file using the following command:
+
+ uv run {script} dependencies clarification-hash \\
+ <crate directory above>/<license file path>
+
+ If a hash has changed, read the file and ensure the license expression still matches.
+ If it does, an updated deny.toml should be committed.
+ """).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(error_line_output),
+ script=SCRIPT_PATH,
+ )
+ )
+
+
+def require_no_stale_clarifications(
+ declared: set[CrateName], evaluated: dict[CrateName, set[CrateVersion]]
+) -> None:
+ """Fail if a clarification names a crate that is not in the dependency graph."""
+ stale: list[CrateName] = sorted(declared - evaluated.keys())
+ if not stale:
+ return
+
+ raise Failure(
+ textwrap.dedent(
+ """
+ {deny_toml} declares license clarifications for crates that cargo-deny did
+ not find in the dependency graph:
+ {crates}
+
+ These are likely stale entries that should be removed.
+ """
+ ).format(
+ deny_toml=DENY_TOML,
+ crates="\n".join(f" {crate_name}" for crate_name in stale),
+ )
+ )
+
+
+def clarification_file_hash(path: Path) -> str:
+ """The hash cargo-deny expects in a [[licenses.clarify]] `license-files` entry.
+
+ XxHash32 with seed 0, over the file with every line terminated by exactly one LF.
+
+ cargo-deny appears to offer no way to compute this value,
+ so this command is added to make it easy (based on the cargo-deny source code).
Review Comment:
**Clarify the hashing helper's provenance** ([licenses.py:323](https://github.com/apache/iceberg-rust/blob/683d60c194d243ccc3cf61946e877d2196212c62/dev/release/licenses.py#L323)). The docstring says it is "based on the cargo-deny source code." Does that mean independently implementing the algorithm, or copying/translating upstream code? If code was copied or adapted, please document its origin and selected upstream license and preserve applicable notices. This is a provenance question, not a confirmed license violation. ASF's policy says: "Do not modify or remove any copyright notices or licenses within third-party works."
##########
deny.toml:
##########
@@ -43,3 +47,33 @@ exceptions = [
{ allow = ["MPL-2.0"], crate = "colored" },
{ allow = ["MPL-2.0"], crate = "option-ext" },
]
+
+# Clarifications use a hash to verify if the license file bytes changed.
+# To recompute: uv run dev/release/licenses.py dependencies clarification-hash <file>
+
+# TODO: zstd-sys 2.1 relicensed the wrapper and bindings to BSD-3-Clause.
+# Transient dependency, drop when we upgrade (likely when we upgrade to Arrow 60).
+# https://github.com/apache/iceberg-rust/issues/3307
+[[licenses.clarify]]
+crate = "zstd-sys"
+expression = "(MIT OR Apache-2.0) AND BSD-3-Clause AND (GPL-2.0-only OR BSD-3-Clause)"
Review Comment:
**Include the embedded BSD-2-Clause code in the zstd clarification** ([deny.toml:59](https://github.com/apache/iceberg-rust/blob/683d60c194d243ccc3cf61946e877d2196212c62/deny.toml#L59)). The packaged legacy decoders contain BSD-2-Clause licenses and Yann Collet copyright notices, for example [zstd_v05.c](https://github.com/facebook/zstd/blob/v1.5.7/lib/legacy/zstd_v05.c#L17). I checked that the packaged source matches upstream v1.5.7 and that the dependency graph enables `zstd-sys/legacy`. Please account for these embedded licenses in the expression and supporting evidence, and preserve the corresponding notices in the planned binary attribution output. ASF requires that every third-party work include its associated license; checking the crate's top-level license files alone misses these notices. See [ASF's third-party source policy](https://www.apache.org/legal/src-headers.html) and [binary distribution guidance](https://infra.apache.org/licensing-howto.html#binary-distributions)
.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6238609350
do we really need these clarifications? im worried about the maintenance burden of adding this PR.
From what I've gathered, we're doing this for the python wheel attribution. Could we take a look at datafusion-python and follow its model?
> **datafusion-python:** it uses `dev/create_license.py`, which runs `cargo-license` to generate a dependency table appended to the root `LICENSE.txt` (202 lines, committed). It also lists names and licenses rather than full texts.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6265656422
> From what I've gathered, we're doing this for the python wheel attribution. Could we take a look at datafusion-python and follow its model?
>
> > **datafusion-python:** it uses `dev/create_license.py`, which runs `cargo-license` to generate a dependency table appended to the root `LICENSE.txt` (202 lines, committed). It also lists names and licenses rather than full texts.
So, I'm a bit cautious of looking too closely at other implementations, because I think copying those over may not yield full compliance with the license obligations. For instance, for Apache 2.0 licenses we must copy all the relevant `NOTICE` files into one final `NOTICE` file, which the DataFusion scripts don't appear to handle today.
My plan here is to use `cargo-about` where we will have a small handful of similar clarifications, but it will meet the obligations we're aware of.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260780066
##########
dev/release/licenses.py:
##########
@@ -68,6 +351,12 @@ def command_check_dependencies(_args: argparse.Namespace) -> None:
if result.returncode != 0:
raise Failure("cargo-deny rejected the dependency licenses.")
+ declared = declared_clarifications()
+ evaluated, clarified = evaluated_crates()
+
+ require_clarifications_applied(declared, evaluated, clarified)
+ require_no_stale_clarifications(declared, evaluated)
Review Comment:
https://github.com/apache/iceberg-rust/pull/3414#discussion_r4250419799
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "blackmwk (via GitHub)" <gi...@apache.org>.
blackmwk commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6211157485
Reviewed at 683d60c194d243ccc3cf61946e877d2196212c62. I found one omission in the license clarification and one provenance question:
1. **Include the embedded BSD-2-Clause code in the zstd clarification** ([deny.toml:59](https://github.com/apache/iceberg-rust/blob/683d60c194d243ccc3cf61946e877d2196212c62/deny.toml#L59)). The packaged legacy decoders contain BSD-2-Clause licenses and Yann Collet copyright notices, for example [zstd_v05.c](https://github.com/facebook/zstd/blob/v1.5.7/lib/legacy/zstd_v05.c#L17). I checked that the packaged source matches upstream v1.5.7 and that the dependency graph enables `zstd-sys/legacy`. Please account for these embedded licenses in the expression and supporting evidence, and preserve the corresponding notices in the planned binary attribution output. ASF requires that every third-party work include its associated license; checking the crate's top-level license files alone misses these notices. See [ASF's third-party source policy](https://www.apache.org/legal/src-headers.html) and [binary distribution guidance](https://infra.apache.org/licensing-howto.html#binary-distributio
ns).
2. **Clarify the hashing helper's provenance** ([licenses.py:323](https://github.com/apache/iceberg-rust/blob/683d60c194d243ccc3cf61946e877d2196212c62/dev/release/licenses.py#L323)). The docstring says it is "based on the cargo-deny source code." Does that mean independently implementing the algorithm, or copying/translating upstream code? If code was copied or adapted, please document its origin and selected upstream license and preserve applicable notices. This is a provenance question, not a confirmed license violation. ASF's policy says: "Do not modify or remove any copyright notices or licenses within third-party works."
The `GPL-2.0-only OR BSD-3-Clause` alternative is compatible with selecting BSD under [ASF's license-choice policy](https://www.apache.org/legal/resolved.html). Likewise, the Brotli `BSD-3-Clause AND MIT` expression correctly represents cumulative obligations; the planned attribution follow-up still needs to provide the required texts and notices in distributed wheels.
Validation: the dependency check passes with CI-pinned `cargo-deny 0.19.9`, and all six configured hashes match the crate sources. Additional probes correctly rejected mismatching hashes, stale clarifications, disallowed license expressions, version pins, and a clarification missing from only one evaluated version.
---
AI-assisted review, posted at the maintainer's request. Please challenge any finding that appears incorrect.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones closed pull request #3414: fix(cargo-deny): clarify zstd-sys and brotli-decompressor
URL: https://github.com/apache/iceberg-rust/pull/3414
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260745125
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
Review Comment:
Omitted intentionally: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4260733893
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4285037035
##########
dev/release/licenses.py:
##########
@@ -56,6 +75,270 @@ def require_cargo_deny() -> None:
)
+def declared_clarifications() -> set[CrateName]:
+ """Crate names that deny.toml carries a [[licenses.clarify]] block for."""
+ with DENY_TOML.open("rb") as handle:
+ config = tomllib.load(handle)
+
+ blocks = config.get("licenses", {}).get("clarify", [])
+ if not blocks:
+ raise Failure(
+ f"Expected at least one [[licenses.clarify]] block in {DENY_TOML}, found none.\n"
+ "If the last one was deliberately removed, remove this check with it."
+ )
+
+ pinned = sorted(block["crate"] for block in blocks if "@" in block["crate"])
+ if pinned:
+ raise Failure(
+ textwrap.dedent("""
+ {deny_toml} pins a [[licenses.clarify]] block to a crate version:
+ {specs}
+
+ Version pins are rejected by this script.
+ cargo-deny supports them, but will not report when they become stale.
+ To keep things simple, this script requires that no pinning is used.
+ Where newer versions change licensing, the script will reject the clarification
+ and the deny.toml file should be updated.
+ """).format(
+ deny_toml=DENY_TOML,
+ specs="\n".join(f" {spec}" for spec in pinned),
+ )
+ )
+
+ return {block["crate"] for block in blocks}
+
+
+def evaluated_crates() -> tuple[
+ dict[CrateName, set[CrateVersion]], dict[CrateName, set[CrateVersion]]
+]:
+ """Every crate cargo-deny evaluated, along with the subset that had a clarification applied."""
+ result = subprocess.run(
+ [
+ "cargo",
+ "deny",
+ "--format=json",
+ "check",
+ "license",
+ # We emit the license accepted messages as warnings so we can count them.
+ "--warn=accepted",
+ ],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ diagnostics = result.stderr
+ if result.returncode != 0:
+ print(diagnostics, file=sys.stderr, end="")
+ raise Failure(
+ "cargo-deny failed while re-running to verify that no license "
+ "clarifications are ignored."
+ )
+
+ evaluated: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ clarified: dict[CrateName, set[CrateVersion]] = defaultdict(set)
+ for line in diagnostics.splitlines():
+ if not line.strip():
+ continue
+ line_truncated = line[:200]
+ try:
+ diagnostic = json.loads(line)
+ except json.JSONDecodeError:
+ raise Failure(
+ "cargo-deny emitted a line that is not JSON, so its output cannot be "
+ f"parsed:\n {line_truncated}"
+ )
+
+ fields = diagnostic.get("fields", {})
+
+ # Anything not accepted should already be reported.
+ if fields.get("code") != "accepted":
+ continue
+
+ graphs = fields.get("graphs") or []
+ # An `accepted` verdict covers exactly one crate. Other checks attach several.
+ if len(graphs) != 1:
+ raise Failure(
+ f"cargo-deny reported an 'accepted' license verdict covering "
+ f"{len(graphs)} crates, but one was expected:\n {line_truncated}"
+ )
+ krate = graphs[0].get("Krate", {})
+
+ name, version = krate.get("name"), krate.get("version")
+ if name is None or version is None:
+ raise Failure(
+ "cargo-deny reported an 'accepted' license verdict without naming both "
+ f"the crate and its version:\n {line_truncated}"
+ )
+
+ evaluated[name].add(version)
+ labels = fields.get("labels") or []
+ CLARIFICATION_APPLIED_MSG = "license expression retrieved via user override"
+ if any(label.get("message") == CLARIFICATION_APPLIED_MSG for label in labels):
Review Comment:
There is no 'stable indicator', this is the best we have for now.
This fails open - so if we can no longer detect clarifications applied, then the script will fail and we can adjust. I'll add this as a code comment so its obvious.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6202916317
@CTTY or @kevinjqliu, please would you be able to take a pass and maybe merge this one?
Then there's one last PR to actually generate the attribution notices. From there, we can focus on 0.11!
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "dannycjones (via GitHub)" <gi...@apache.org>.
dannycjones commented on code in PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#discussion_r4293430239
##########
deny.toml:
##########
@@ -43,3 +47,33 @@ exceptions = [
{ allow = ["MPL-2.0"], crate = "colored" },
{ allow = ["MPL-2.0"], crate = "option-ext" },
]
+
+# Clarifications use a hash to verify if the license file bytes changed.
+# To recompute: uv run dev/release/licenses.py dependencies clarification-hash <file>
+
+# TODO: zstd-sys 2.1 relicensed the wrapper and bindings to BSD-3-Clause.
+# Transient dependency, drop when we upgrade (likely when we upgrade to Arrow 60).
+# https://github.com/apache/iceberg-rust/issues/3307
+[[licenses.clarify]]
+crate = "zstd-sys"
+expression = "(MIT OR Apache-2.0) AND BSD-3-Clause AND (GPL-2.0-only OR BSD-3-Clause)"
Review Comment:
Nice catch - wow this complicates things :)
I've found a way to do the attribution in the later PR, not too complicated. I'll update this expression now.
I will need to hash the C file itself.
The C file's content does change from time to time (twice last year: https://github.com/facebook/zstd/commits/dev/lib/legacy/zstd_v05.c) - I think it will be a necessary cost to revalidate when we update zstd unfortunately.
Note, I will include only the one file for now. I think its important that we understand where we can remove clarifications when the license no longer applies. New licenses appearing is a moving target which we can't reliably verify when they're not declared.
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org
Re: [PR] fix(cargo-deny): clarify zstd-sys and brotli-decompressor [iceberg-rust]
Posted by "kevinjqliu (via GitHub)" <gi...@apache.org>.
kevinjqliu commented on PR #3414:
URL: https://github.com/apache/iceberg-rust/pull/3414#issuecomment-6301103043
looks like we can drop the zstd requirement by just removing the feature: https://github.com/apache/iceberg-rust/pull/3495
--
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: issues-unsubscribe@iceberg.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
---------------------------------------------------------------------
To unsubscribe, e-mail: issues-unsubscribe@iceberg.apache.org
For additional commands, e-mail: issues-help@iceberg.apache.org