rangareddy commented on code in PR #857:
URL: https://github.com/apache/incubator-xtable/pull/857#discussion_r3718152430
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -471,7 +579,15 @@ def render_notice(module: pathlib.Path) -> str:
def main() -> None:
+ missing_texts: dict[pathlib.Path, list[tuple[str, str, str]]] = {}
for module in shade_modules():
+ if not (module / "target" / "dependency-tree-runtime.txt").exists():
Review Comment:
Fixed, and the impact turned out to be wider than the abort.
The version is now parsed as `parts[-2]`, with the classifier read only when
the line has six fields, and a `tests` classifier excluded alongside `test-jar`
(`org.apache.hbase:hbase-common:jar:tests:2.0.0-alpha4` was getting through,
since its *type* is `jar`).
Beyond the crash: there are ten classified coordinates across the four
trees, and the nine that did not raise were being written into
`LICENSE-bundled` with the **classifier in place of the version** —
`io.netty:netty-transport-native-epoll:linux-aarch_64`,
`org.apache.orc:orc-core:shaded-protobuf`, `org.apache.hive:hive-exec:core`.
They survived only because an override short-circuited `normalize_family()`
before the POM lookup, which means six of the `ARTIFACT_OVERRIDES` existed
purely to mask this bug. All six are now gone (see the override thread).
On the skip list: I was going to argue against it and the numbers changed my
mind, so I have taken your suggestion. Covering `xtable-utilities` needs ~32
curated license texts plus a family decision for
`org.openjdk.jol:jol-core:0.16`, which is GPLv2-with-Classpath-Exception and
deliberately shaded (`xtable-utilities/pom.xml:717`). For an artifact
AGENTS.md:105 calls unpublished and not release-blocking, that is not rc2 work.
`SKIPPED_SHADE_MODULES` now mirrors the validator's list with a comment saying
why. I did still add the GPLv2+CPE family mapping and a `FAMILY_ORDER` entry,
so the module can be un-skipped later without hitting an unmapped family.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -471,7 +579,15 @@ def render_notice(module: pathlib.Path) -> str:
def main() -> None:
+ missing_texts: dict[pathlib.Path, list[tuple[str, str, str]]] = {}
for module in shade_modules():
+ if not (module / "target" / "dependency-tree-runtime.txt").exists():
Review Comment:
Agreed, and made stricter than requested: rather than regenerating the
modules that do have a tree and exiting non-zero at the end, the run now aborts
**before writing anything**, so it is a genuine no-op. The error names the
modules and prints the exact `./mvnw … dependency:tree` command to produce them.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -447,6 +475,86 @@ def notice_text_for(group: str, artifact: str, version:
str) -> str | None:
return None
+@lru_cache(maxsize=None)
+def license_text_from_jar(group: str, artifact: str, version: str) -> str |
None:
+ path = jar_path(group, artifact, version)
+ if not path.exists():
+ return None
+ with zipfile.ZipFile(path) as jar_file:
+ names = set(jar_file.namelist())
+ for candidate in JAR_LICENSE_CANDIDATES:
+ if candidate not in names:
+ continue
+ text = jar_file.read(candidate).decode("utf-8", errors="replace")
+ lines = [line.rstrip() for line in text.replace("\r\n",
"\n").replace("\r", "\n").split("\n")]
+ normalized = "\n".join(lines).strip()
+ if normalized:
+ return normalized + "\n"
+ return None
+
+
+def license_text_for(group: str, artifact: str, version: str) -> str | None:
+ """Return the license text for a bundled dependency.
+
+ A curated override keyed by artifactId is consulted first: it is human
+ verified and is required for dependencies whose jar carries no license, or
a
+ misleading one (e.g. junit bundles Hamcrest's BSD text rather than its own
+ Common Public License). Otherwise the dependency's own jar is
authoritative.
+ Returns None when neither source has a text, so the caller can fail loudly
+ rather than ship an unattributed dependency.
+ """
+ override = OVERRIDES_DIR / f"LICENSE-{artifact}"
+ if override.exists():
+ content = override.read_text()
+ return content if content.endswith("\n") else content + "\n"
+
+ return license_text_from_jar(group, artifact, version)
+
+
+def bundled_non_apache_coords(license_path: pathlib.Path) -> list[tuple[str,
str, str]]:
+ """Parse the non-Apache dependency coordinates out of a LICENSE-bundled
file.
+
+ License texts are keyed off the committed LICENSE-bundled (the canonical
list
+ of what the shaded jar bundles) rather than a freshly resolved dependency
+ tree, so the texts stay reproducible regardless of the local ~/.m2 state.
+ """
+ coords: list[tuple[str, str, str]] = []
+ family = None
+ lines = license_path.read_text().splitlines()
+ for index, line in enumerate(lines):
+ stripped = line.strip()
+ if not stripped:
+ family = None
+ continue
+ if index + 1 < len(lines) and re.fullmatch(r"-{3,}", lines[index +
1].strip()):
+ family = stripped
+ continue
+ if re.fullmatch(r"-{3,}", stripped):
+ continue
+ if family and family != APACHE_FAMILY and stripped.count(":") == 2:
+ group, artifact, version = stripped.split(":")
+ coords.append((group, artifact, version))
+ return coords
+
+
+def sync_license_texts(module: pathlib.Path, coords: list[tuple[str, str,
str]]) -> list[tuple[str, str, str]]:
+ """Regenerate META-INF/licenses/ for a module; return coords with no
text."""
+ licenses_dir = module / "src" / "main" / "resources" / "META-INF" /
"licenses"
+ licenses_dir.mkdir(parents=True, exist_ok=True)
+
+ for stale in licenses_dir.glob("LICENSE-*"):
Review Comment:
Done, and at the whole-run level rather than per-module, which is the
"ideally" version you described.
- `plan_module()` computes everything a module would write and writes
nothing.
- `resolve_license_texts()` returns `(resolved, missing, mismatched)` with
no `unlink()` anywhere near it.
- `main()` plans every module first; only if all of them resolve cleanly
does it write. `write_license_texts()` is now the only function that deletes,
and it runs after all resolution.
So `LICENSE-bundled`/`NOTICE-bundled` are deferred too, and a failing run
leaves the tree byte-identical. Verified repeatedly while working through these
comments: every failure left `git status` showing only my own source edits.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -447,6 +475,86 @@ def notice_text_for(group: str, artifact: str, version:
str) -> str | None:
return None
+@lru_cache(maxsize=None)
+def license_text_from_jar(group: str, artifact: str, version: str) -> str |
None:
+ path = jar_path(group, artifact, version)
+ if not path.exists():
+ return None
+ with zipfile.ZipFile(path) as jar_file:
+ names = set(jar_file.namelist())
+ for candidate in JAR_LICENSE_CANDIDATES:
+ if candidate not in names:
+ continue
+ text = jar_file.read(candidate).decode("utf-8", errors="replace")
+ lines = [line.rstrip() for line in text.replace("\r\n",
"\n").replace("\r", "\n").split("\n")]
+ normalized = "\n".join(lines).strip()
+ if normalized:
+ return normalized + "\n"
+ return None
+
+
+def license_text_for(group: str, artifact: str, version: str) -> str | None:
+ """Return the license text for a bundled dependency.
+
+ A curated override keyed by artifactId is consulted first: it is human
+ verified and is required for dependencies whose jar carries no license, or
a
+ misleading one (e.g. junit bundles Hamcrest's BSD text rather than its own
+ Common Public License). Otherwise the dependency's own jar is
authoritative.
+ Returns None when neither source has a text, so the caller can fail loudly
+ rather than ship an unattributed dependency.
+ """
+ override = OVERRIDES_DIR / f"LICENSE-{artifact}"
+ if override.exists():
+ content = override.read_text()
+ return content if content.endswith("\n") else content + "\n"
+
+ return license_text_from_jar(group, artifact, version)
+
+
+def bundled_non_apache_coords(license_path: pathlib.Path) -> list[tuple[str,
str, str]]:
+ """Parse the non-Apache dependency coordinates out of a LICENSE-bundled
file.
+
+ License texts are keyed off the committed LICENSE-bundled (the canonical
list
+ of what the shaded jar bundles) rather than a freshly resolved dependency
+ tree, so the texts stay reproducible regardless of the local ~/.m2 state.
+ """
+ coords: list[tuple[str, str, str]] = []
+ family = None
+ lines = license_path.read_text().splitlines()
+ for index, line in enumerate(lines):
+ stripped = line.strip()
+ if not stripped:
+ family = None
+ continue
+ if index + 1 < len(lines) and re.fullmatch(r"-{3,}", lines[index +
1].strip()):
+ family = stripped
+ continue
+ if re.fullmatch(r"-{3,}", stripped):
+ continue
+ if family and family != APACHE_FAMILY and stripped.count(":") == 2:
+ group, artifact, version = stripped.split(":")
+ coords.append((group, artifact, version))
+ return coords
+
+
+def sync_license_texts(module: pathlib.Path, coords: list[tuple[str, str,
str]]) -> list[tuple[str, str, str]]:
+ """Regenerate META-INF/licenses/ for a module; return coords with no
text."""
+ licenses_dir = module / "src" / "main" / "resources" / "META-INF" /
"licenses"
+ licenses_dir.mkdir(parents=True, exist_ok=True)
+
+ for stale in licenses_dir.glob("LICENSE-*"):
Review Comment:
Reproduced exactly, down to the same five files — `LICENSE-activation`,
`LICENSE-commons-el`, `LICENSE-mail`, `LICENSE-oro`, `LICENSE-protobuf-java`,
with `LICENSE-bundled` and `NOTICE-bundled` rewritten. Thanks for the precise
report; it made this easy to confirm.
Fixed as described in the thread above: resolution now completes for every
module before anything is written or unlinked, so a failed run is a no-op
across all modules and the script is safe to re-run.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -483,6 +599,21 @@ def main() -> None:
license_path.write_text(render_license(groups))
notice_path.write_text(render_notice(module))
+ missing = sync_license_texts(module,
bundled_non_apache_coords(license_path))
Review Comment:
The round-trip is gone — `plan_module()` takes the non-Apache coordinates
straight from the `groups` dict, exactly as you wrote it.
One deviation: I kept `bundled_non_apache_coords()` rather than deleting it,
because your other comment asks for a single shared parser and the validator
does have to read the committed file. It is no longer the generator's input
path; it is now the one implementation, exposed as `--non-apache-artifact-ids`
and called from `validate_shaded_license_coverage.sh` in place of the awk
block. That way both comments are satisfied at once instead of trading one for
the other, and the three parsers you counted are down to one — the `^-+$` vs
`-{3,}` divergence you spotted can no longer happen.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -447,6 +475,86 @@ def notice_text_for(group: str, artifact: str, version:
str) -> str | None:
return None
+@lru_cache(maxsize=None)
+def license_text_from_jar(group: str, artifact: str, version: str) -> str |
None:
+ path = jar_path(group, artifact, version)
+ if not path.exists():
+ return None
+ with zipfile.ZipFile(path) as jar_file:
+ names = set(jar_file.namelist())
+ for candidate in JAR_LICENSE_CANDIDATES:
+ if candidate not in names:
+ continue
+ text = jar_file.read(candidate).decode("utf-8", errors="replace")
Review Comment:
Switched to strict, and to answer your follow-up directly — **yes, it would
have broken today, on two dependencies, and after the classified-artifact fix
it no longer does.**
I decoded all 363 embedded `LICENSE*`/`NOTICE*` entries reachable from the
three release modules. Four artifacts carry non-UTF-8 bytes:
| coordinate | entry | failure |
|---|---|---|
| `javax.activation:activation:1.1` | `META-INF/LICENSE.txt` | `0xe1` at
2427 |
| `javax.mail:mail:1.4.1` | `META-INF/LICENSE.txt` | `0xba` at 14369 |
| `javax.transaction:jta:1.1`, `transaction-api:1.1` |
`META-INF/LICENSE.txt` | `0xe1` at 2393 |
| `org.apache.ant:ant:1.9.1`, `ant-launcher:1.9.1` | `META-INF/LICENSE.txt`
| `0xae` at 11770 |
Reachability per path:
- `notice_text_for()` (all coordinates): **0** failures, so the pre-existing
`errors="replace"` you flagged at line 470 was harmless in practice. It is
strict now as well.
- `license_text_from_jar()` (non-Apache only): the two `javax.*` ones. `jta`
and `ant` normalise to Apache 2.0 and never reach it.
- Once the bundled set is computed correctly, `javax.activation` and
`javax.mail` are no longer bundled at all, so the current run has zero decode
failures.
Your instinct was right; the specific artifacts were
`javax.activation`/`javax.mail` rather than jline/xmlenc/oro. Implemented as
`_decode_entry()` raising `UndecodableText`: for license texts that routes the
artifact onto the "needs a curated override" list, and NOTICE failures are
collected and reported rather than silently mangled into U+FFFD.
##########
release/scripts/generate_shaded_license_metadata.py:
##########
@@ -277,6 +302,7 @@
"javax.servlet": "Apache License 2.0",
"javax.servlet.jsp": "Apache License 2.0",
"javax.transaction": "Apache License 2.0",
+ "org.apache.hadoop": "Apache License 2.0",
Review Comment:
Confirmed — you were right, it is not required, and it is now deleted. But
it took closing two separate POM-parsing gaps to make that true, and neither
was a `~/.m2` miss.
**Gap 1 — a POM expat rejects.** `hadoop-yarn-api:3.1.0` inherits from
`hadoop-yarn:3.1.0` → `hadoop-project:3.1.0`, and that file fails to parse at
line 1787:
```xml
<Xlint:-unchecked/>
```
`Xlint` is an undeclared namespace prefix and the local name starts with a
hyphen. Maven's parser accepts it; expat rejects the whole file. The POM is
authentic, not a corrupt download — SHA-1 matches its `.sha1` sidecar from
Central (`f991e7f6d388e33a3fe83e18cfbf8815f7222e75`). Its own parent
`hadoop-main:3.1.0` declares Apache 2.0 and parses fine, so the answer was
always one hop away.
**Gap 2 — the Maven namespace bound to a prefix.**
`hadoop-yarn-client-2.7.1.pom` opens with
```xml
<project xmlns:pom="http://maven.apache.org/POM/4.0.0";>
```
so every element is unqualified and `./m:parent` / `./m:licenses` matched
nothing — the chain looked empty. Also authentic per its SHA-1.
Fixes: POM lookups now match on **local name** rather than a namespaced
XPath, and `_lenient_pom_scan()` recovers the license names and parent
coordinate by text scan when expat refuses a file. With both in place every
hadoop coordinate resolves to Apache 2.0 from its own POM chain, so the
override is gone.
**On your second hypothesis** — that a resolution failure would be silently
mislabelling other coordinates — it does not hold. `normalize_family()` ends in
`raise ValueError("Unmapped license for …")`, and neither `__MISSING__` nor
`__PARSE_ERROR__` matches any marker, so an unresolvable POM raises rather than
defaulting to anything. The fail-loud behaviour you wanted is already there;
the `hudi-io` crash was it working.
**While I was in there**, I audited every override against a fresh
resolution and dropped 18 that only repeated what the POM says. Each traces to
one of the three bugs:
| bug | overrides it made unnecessary |
|---|---|
| expat rejects the POM | `org.apache.hadoop` |
| namespace not the default (12 bundled POMs hit this) | `aopalliance`,
`it.unimi.dsi`, `javax.inject`, `org.codehaus.jackson`, `xml-apis`, `xmlenc` |
| classifier read as version | `io.netty`, `org.apache.hbase`, and all six
`ARTIFACT_OVERRIDES` |
`xml-apis` was not merely redundant but **wrong**: it forced `Apache
Software License 1.1` while both the POM and the jar's own `license/LICENSE`
carry the Apache **2.0** text. `ARTIFACT_OVERRIDES` is down to one entry
(`bcprov-jdk18on`, whose POM says "Bouncy Castle Licence" — the MIT text
verbatim), with the reason in a comment.
Three overrides I deliberately left alone, because they change ASF category
and that is a legal call rather than a scripting one:
| override | says | POM says |
|---|---|---|
| `javax.servlet` | Apache License 2.0 | CDDL + GPLv2 w/ CPE |
| `javax.servlet.jsp` | Apache License 2.0 | CDDL + GPLv2 w/ CPE |
| `stax` | CDDL + GPLv2 w/ CPE | Apache License 2.0 |
The first two downgrade Category B to Category A, which suppresses the
license text those dependencies would otherwise ship. Worth your second opinion.
##########
release/scripts/validate_shaded_license_coverage.sh:
##########
@@ -254,6 +254,38 @@ for module in "${SHADE_MODULES[@]}"; do
printf ' - %s\n' "${unknown[@]}"
overall_status=1
fi
+
+ # Every dependency bundled under a non-Apache-2.0 license must ship its own
+ # license text under META-INF/licenses/LICENSE-<artifactId>. The Apache
banner
+ # at the top of LICENSE-bundled only covers the Apache License 2.0 family.
+ licenses_dir="${module}/src/main/resources/META-INF/licenses"
Review Comment:
Added, essentially as you wrote it, and verified that it fires:
```
$ echo planted >
xtable-aws/src/main/resources/META-INF/licenses/LICENSE-not-a-real-dep
$ release/scripts/validate_shaded_license_coverage.sh
FAIL xtable-aws: orphaned META-INF/licenses/ text with no bundled
non-Apache-2.0 dependency:
- LICENSE-not-a-real-dep
```
The success line now reads "license texts match the bundled non-Apache-2.0
dependencies **exactly**", since it asserts both directions. I hoisted the
artifactId list into a variable as you suggested, though it now comes from the
shared Python parser rather than the awk block (see the other thread).
One correction to the framing, so nobody is misled later: the sets are not
identical. A full regeneration removes nine orphans — `LICENSE-activation`,
`asm`, `asm-commons`, `asm-tree`, `commons-el`, `mail`, `oro`, `protobuf-java`,
`xml-apis` — whereas #866 removes `LICENSE-antlr4-runtime`,
`LICENSE-slf4j-api`, `LICENSE-javax-annotation-api` and
`LICENSE-glassfish-java-servlet-jsp`, which an earlier commit on this branch
already removed. So #866 is genuinely subsumed and can be closed, but as
"superseded", not "the same six files".
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
