vinishjail97 commented on code in PR #857:
URL: https://github.com/apache/incubator-xtable/pull/857#discussion_r3716205913
##########
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:
This unlinks the module's existing texts before it knows a replacement can
be produced for every coordinate, and the fail-loud check is at line 615 —
after the loop has already written `LICENSE-bundled`, `NOTICE-bundled`, and
whatever subset of texts did resolve, for *every* module.
So a run that hits a dependency with no jar license and no override leaves
the working tree in a worse state than it started: texts deleted, metadata
rewritten, and a `SystemExit`. The only recovery is `git checkout -- .`, which
also throws away any legitimate regeneration in the same run. For a script that
a release manager runs against the tree that becomes the release artifact, that
is the wrong failure mode.
Resolve first, then write:
```python
def sync_license_texts(module, coords):
resolved = {}
missing = []
for group, artifact, version in coords:
text = license_text_for(group, artifact, version)
if text is None:
missing.append((group, artifact, version))
else:
resolved[f"LICENSE-{artifact}"] = text
if missing:
return missing # nothing written, nothing deleted
licenses_dir = module / "src" / "main" / "resources" / "META-INF" /
"licenses"
licenses_dir.mkdir(parents=True, exist_ok=True)
for stale in licenses_dir.glob("LICENSE-*"):
stale.unlink()
for name, text in resolved.items():
(licenses_dir / name).write_text(text)
return []
```
Ideally the `LICENSE-bundled`/`NOTICE-bundled` writes in `main()` would be
deferred the same way, so a failing run is a genuine no-op across all modules.
##########
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:
`main()` already holds `groups: dict[family, list[coord]]` in memory — it is
what `render_license(groups)` on the previous lines was built from. Writing
that dict to a file and then parsing the file back to recover the same
coordinates is a round-trip we do not need, and it couples the reader to the
exact text layout of the writer.
```python
non_apache = [coord for family, coords in groups.items()
if family != APACHE_FAMILY for coord in coords]
missing = sync_license_texts(module, non_apache)
```
That deletes `bundled_non_apache_coords` entirely. Worth doing because the
same format is currently parsed in **three** places: `render_license` writes
it, `bundled_non_apache_coords` reads it, and the awk block at
`validate_shaded_license_coverage.sh:269` reads it again with slightly
different rules (`^-+$` in awk vs `-{3,}` in Python, so a 1- or 2-dash line is
a section underline to one parser and a coordinate line to the other). Three
parsers for one format is three chances to drift.
The docstring's reasoning — that keying off the committed list keeps output
reproducible regardless of `~/.m2` state — is right, but `groups` in this loop
*is* the committed list at this point, since `render_license(groups)` just
produced it.
##########
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:
What made this override necessary? Hadoop's poms — including 2.7.x, which is
what `xtable-hive-metastore` pulls — declare `Apache License, Version 2.0`, and
`normalize_family` already matches that string. So the family should resolve
correctly without an override.
If the reason is that the hadoop poms were not present in the local `~/.m2`
when you ran the generator, then the override is masking a resolution failure,
and that same failure would be silently mislabelling *other* coordinates in the
same run (they would fall through to whatever the no-pom default is). That is
worth knowing about, because the whole point of this tooling is that a
mislabelled family means a missing or wrong license text.
Could you confirm which it is? If it is a genuine pom quirk (e.g. one
specific hadoop module with no `<licenses>` block), a comment naming that
module would be useful. If it is a `~/.m2` miss, I would rather the script
raised on an unresolvable pom than defaulted.
##########
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:
Will errors="strict" break this script today?
##########
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:
I don't think the hadoop change is required in this script..please confirm?
##########
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:
`errors="replace"` silently substitutes U+FFFD for any byte that is not
valid UTF-8. Several of the older jars in the hive-metastore set (jline,
xmlenc, oro-era artifacts) predate UTF-8 being universal, and a latin-1 `©` in
a copyright line becomes `\ufffd` in the text we then ship as that dependency's
license.
Keep `errors="strict"` and let it raise, so the artifact goes on the "needs
a curated override" list instead — that fits the fail-loud design of the rest
of this change.
Same applies to the existing `notice_text_for` at line 470, though that is
pre-existing?
##########
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:
This new check is one-directional: it catches a bundled non-AL2 dependency
with no text. It does not catch the reverse condition, a `LICENSE-<x>` text
with no corresponding bundled dependency which is precisely #865, and #866 is
currently fixing.
Since you are already walking `licenses_dir` and already have the set of
non-AL2 artifactIds from `LICENSE-bundled`, the orphan check is a few lines on
the data you have:
```bash
orphaned_license_texts=()
while IFS= read -r -d '' text_file; do
artifact_id="$(basename "${text_file}")"
artifact_id="${artifact_id#LICENSE-}"
grep -qxF "${artifact_id}" <<< "${non_apache_artifact_ids}" ||
orphaned_license_texts+=("${artifact_id}")
done < <(find "${licenses_dir}" -name 'LICENSE-*' -print0 2>/dev/null)
```
(reusing the awk output you already compute, hoisted into a variable rather
than piped straight into the loop).
That closes #700 and #865 with one validator and means neither gap can
reopen. It would also let #866 be closed as redundant, since this PR already
deletes all six of its files plus the two it misses.
##########
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:
This prints and continues, so a run against a partially-built tree
regenerates the modules that happen to have a dependency tree and silently
leaves the others at whatever is committed. The output is a `git diff` that
looks like a complete regeneration but is not.
Given this drives release artifacts, I would rather it refused: collect the
skipped modules and exit non-zero at the end (alongside the `missing_texts`
report)?
--
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]
