dannycjones commented on code in PR #3528:
URL: https://github.com/apache/iceberg-rust/pull/3528#discussion_r4378344527
##########
dev/release/licenses.py:
##########
@@ -39,6 +45,122 @@ def ok(message: str) -> None:
print(f"OK: {message}", flush=True)
+def report_error(message: str) -> None:
+ # GitHub Actions turns `::error::` lines into annotations on the job
summary.
+ if os.environ.get("GITHUB_ACTIONS") == "true":
+ print(f"::error::{message}", flush=True)
+ else:
+ print(f"ERROR: {message}", file=sys.stderr, flush=True)
+
+
+def require_cargo() -> None:
+ """Assert cargo is installed."""
+ if shutil.which("cargo") is None:
+ raise Failure("This script requires 'cargo', but it is not installed.")
+
+
+def is_non_empty_file(path: Path) -> bool:
+ """Whether the path resolves, following symlinks, to a non-empty regular
file."""
+ return path.is_file() and path.stat().st_size > 0
+
+
+def publishable_crates() -> list[tuple[str, Path]]:
+ """Name and directory of every crate that would be published in a
release."""
+ result = subprocess.run(
+ ["cargo", "metadata", "--no-deps", "--format-version", "1"],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ sys.stderr.write(result.stderr)
+ raise Failure("Could not list the publishable crates.")
+
+ # `publish` is an empty list for `publish = false` crates and null when the
+ # crate may be published to any registry.
+ return [
+ (package["name"], Path(package["manifest_path"]).parent)
+ for package in json.loads(result.stdout)["packages"]
+ if package["publish"] != []
+ ]
+
+
+def check_crate_files(crate: str, crate_dir: Path) -> bool:
+ """Check the packaged crate ships LICENSE and NOTICE resolving to the root
files."""
+ print(f"Checking {crate}...", flush=True)
+ # --no-verify: nothing is built, only the packaged files are listed.
+ # --allow-dirty: allows `make check` during development.
+ result = subprocess.run(
+ [
+ "cargo",
+ "package",
+ "--package",
+ crate,
+ "--list",
+ "--locked",
+ "--no-verify",
+ "--allow-dirty",
+ ],
+ cwd=REPO_ROOT,
+ stdout=subprocess.PIPE,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ report_error(f"{crate} could not be listed by cargo; see the error
above")
+ return False
+
+ packaged_paths = set(result.stdout.splitlines())
+ passed = True
+ for file_name in REQUIRED_CRATE_FILES:
+ repo_path = crate_dir / file_name
+ if file_name not in packaged_paths:
+ report_error(f"{crate} crate is missing {file_name} in packaged
file list")
+ passed = False
+ elif not is_non_empty_file(repo_path):
+ report_error(f"{crate} {file_name} does not resolve to a non-empty
file: {repo_path}")
+ passed = False
+ elif not repo_path.samefile(REPO_ROOT / file_name):
+ report_error(
+ f"{crate} {file_name} does not resolve to the repository root
{file_name}: {repo_path}"
+ )
+ passed = False
Review Comment:
This will fail the script immediately if the root file doesn't exist.
https://docs.python.org/3/library/pathlib.html#pathlib.Path.samefile
Small regression over the shell script. We can just have a helper like
`is_non_empty_file` which will return false when the file doesn't exist?
##########
dev/release/licenses.py:
##########
@@ -39,6 +45,122 @@ def ok(message: str) -> None:
print(f"OK: {message}", flush=True)
+def report_error(message: str) -> None:
+ # GitHub Actions turns `::error::` lines into annotations on the job
summary.
+ if os.environ.get("GITHUB_ACTIONS") == "true":
+ print(f"::error::{message}", flush=True)
+ else:
+ print(f"ERROR: {message}", file=sys.stderr, flush=True)
+
+
+def require_cargo() -> None:
+ """Assert cargo is installed."""
+ if shutil.which("cargo") is None:
+ raise Failure("This script requires 'cargo', but it is not installed.")
+
+
+def is_non_empty_file(path: Path) -> bool:
+ """Whether the path resolves, following symlinks, to a non-empty regular
file."""
+ return path.is_file() and path.stat().st_size > 0
+
+
+def publishable_crates() -> list[tuple[str, Path]]:
+ """Name and directory of every crate that would be published in a
release."""
+ result = subprocess.run(
+ ["cargo", "metadata", "--no-deps", "--format-version", "1"],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ sys.stderr.write(result.stderr)
+ raise Failure("Could not list the publishable crates.")
+
+ # `publish` is an empty list for `publish = false` crates and null when the
+ # crate may be published to any registry.
+ return [
+ (package["name"], Path(package["manifest_path"]).parent)
+ for package in json.loads(result.stdout)["packages"]
+ if package["publish"] != []
+ ]
+
+
+def check_crate_files(crate: str, crate_dir: Path) -> bool:
+ """Check the packaged crate ships LICENSE and NOTICE resolving to the root
files."""
+ print(f"Checking {crate}...", flush=True)
+ # --no-verify: nothing is built, only the packaged files are listed.
+ # --allow-dirty: allows `make check` during development.
+ result = subprocess.run(
+ [
+ "cargo",
+ "package",
+ "--package",
+ crate,
+ "--list",
+ "--locked",
+ "--no-verify",
+ "--allow-dirty",
+ ],
+ cwd=REPO_ROOT,
+ stdout=subprocess.PIPE,
+ text=True,
+ check=False,
+ )
Review Comment:
the comments should be inline.
I think it would be clearer to just omit `--no-verify` at this point,
`--list` implies it.
```suggestion
result = subprocess.run(
[
"cargo",
"package",
"--package",
crate,
"--list",
"--locked",
# allows `make check` during development.
"--allow-dirty",
],
cwd=REPO_ROOT,
stdout=subprocess.PIPE,
text=True,
check=False,
)
```
##########
dev/release/licenses.py:
##########
@@ -87,6 +209,18 @@ def main() -> int:
)
check.set_defaults(func=command_check_dependencies)
+ crates = cmd_groups.add_parser(
+ "crates",
+ help="LICENSE and NOTICE files shipped by every publishable crate",
+ )
+ crates_commands = crates.add_subparsers(dest="command", required=True)
Review Comment:
The help doesn't make much sense here.
```suggestion
crates = cmd_groups.add_parser(
"crates",
help="Sub-commands for working with licensing on crates",
)
crates_commands = crates.add_subparsers(dest="command", required=True)
```
##########
dev/release/licenses.py:
##########
@@ -39,6 +45,122 @@ def ok(message: str) -> None:
print(f"OK: {message}", flush=True)
+def report_error(message: str) -> None:
+ # GitHub Actions turns `::error::` lines into annotations on the job
summary.
+ if os.environ.get("GITHUB_ACTIONS") == "true":
+ print(f"::error::{message}", flush=True)
+ else:
+ print(f"ERROR: {message}", file=sys.stderr, flush=True)
+
+
+def require_cargo() -> None:
+ """Assert cargo is installed."""
+ if shutil.which("cargo") is None:
+ raise Failure("This script requires 'cargo', but it is not installed.")
Review Comment:
Please use the same logic as `require_cargo_deny`. Please also keep the two
functions alongside each other. (Bring `require_cargo_deny` up to here.)
##########
dev/release/licenses.py:
##########
@@ -39,6 +45,122 @@ def ok(message: str) -> None:
print(f"OK: {message}", flush=True)
+def report_error(message: str) -> None:
+ # GitHub Actions turns `::error::` lines into annotations on the job
summary.
+ if os.environ.get("GITHUB_ACTIONS") == "true":
+ print(f"::error::{message}", flush=True)
+ else:
+ print(f"ERROR: {message}", file=sys.stderr, flush=True)
+
+
+def require_cargo() -> None:
+ """Assert cargo is installed."""
+ if shutil.which("cargo") is None:
+ raise Failure("This script requires 'cargo', but it is not installed.")
+
+
+def is_non_empty_file(path: Path) -> bool:
+ """Whether the path resolves, following symlinks, to a non-empty regular
file."""
+ return path.is_file() and path.stat().st_size > 0
+
+
+def publishable_crates() -> list[tuple[str, Path]]:
+ """Name and directory of every crate that would be published in a
release."""
+ result = subprocess.run(
+ ["cargo", "metadata", "--no-deps", "--format-version", "1"],
+ cwd=REPO_ROOT,
+ capture_output=True,
+ text=True,
+ check=False,
+ )
+ if result.returncode != 0:
+ sys.stderr.write(result.stderr)
+ raise Failure("Could not list the publishable crates.")
Review Comment:
I don't think we should capture output, we'll miss warnings. The shell
script wasn't capturing it.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
