Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 35 additions & 0 deletions build_tools/_therock_utils/storage_location.py
Original file line number Diff line number Diff line change
Expand Up @@ -59,3 +59,38 @@ def local_path(self, staging_dir: Path) -> Path:
Full path: ``{staging_dir}/{relative_path}``
"""
return staging_dir / self.relative_path

def cdn_url(
self,
base_url: str,
strip_prefix: str = "",
cdn_prefix: str = "",
) -> str:
"""CDN URL, optionally remapping the storage path prefix to a CDN prefix.

Strips ``strip_prefix`` from the start of ``relative_path`` (if present),
then prepends ``cdn_prefix``, and finally prepends ``base_url``.
Comment on lines +63 to +72
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.


This lets private workflows surface artifacts through a CDN whose path
layout differs from the storage key layout. For example, files stored under
``v3/artifacts/{run_id}-linux/...`` may be served as
``artifacts/{run_id}-linux/...`` on the CDN.

Args:
base_url: CDN base URL without trailing slash
(e.g. ``'https://artifacts.example.com'``).
strip_prefix: Path prefix to strip from ``relative_path``
(e.g. ``'v3/artifacts'``). No-op if empty or not matched.
cdn_prefix: CDN path prefix to prepend after stripping
(e.g. ``'artifacts'``). No-op if empty.

Returns:
Full CDN URL (e.g.
``'https://artifacts.example.com/artifacts/12345-linux/file.tar.xz'``).
"""
path = self.relative_path
if strip_prefix and path.startswith(f"{strip_prefix}/"):
path = path[len(strip_prefix) + 1 :]
if cdn_prefix:
path = f"{cdn_prefix}/{path}"
return f"{base_url}/{path}"
27 changes: 26 additions & 1 deletion build_tools/_therock_utils/workflow_outputs.py
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,17 @@ class WorkflowOutputRoot:
platform: str
"""Platform name ('linux' or 'windows')."""

path_prefix: str = ""
"""Optional storage path prefix prepended before all output paths.

When set (e.g. ``'v3/artifacts'``), all outputs for this run are placed
under ``{path_prefix}/{external_repo}{run_id}-{platform}/...`` instead of
the default ``{external_repo}{run_id}-{platform}/...``.

Useful for private workflows that need to co-locate outputs with other
content under a versioned directory on the same bucket.
"""

# -- Root -------------------------------------------------------------------

@property
Expand All @@ -96,7 +107,10 @@ def prefix(self) -> str:

This is the common root for all outputs from this run.
"""
return f"{self.external_repo}{self.run_id}-{self.platform}"
base = f"{self.external_repo}{self.run_id}-{self.platform}"
if self.path_prefix:
return f"{self.path_prefix}/{base}"
return base

def root(self) -> StorageLocation:
"""Location for the run output root (where build artifacts live)."""
Expand Down Expand Up @@ -236,6 +250,8 @@ def from_workflow_run(
github_repository: str | None = None,
workflow_run: dict | None = None,
lookup_workflow_run: bool = False,
bucket_override: str | None = None,
path_prefix: str = "",
) -> "WorkflowOutputRoot":
"""Create from CI workflow context.

Expand All @@ -255,6 +271,11 @@ def from_workflow_run(
Most callers running inside their own CI workflow do not need
this — environment variables suffice. Set this when looking up
another repository's workflow run (e.g. fetching artifacts).
bucket_override: When set, use this S3 bucket instead of the one
determined from the repository/fork/release_type heuristics.
Bucket permissions are enforced by AWS IAM, not by this code.
path_prefix: Optional storage path prefix for all outputs under this run
(e.g. ``'v3/artifacts'``). See ``WorkflowOutputRoot.path_prefix``.
"""
workflow_run_id = (
run_id if lookup_workflow_run and workflow_run is None else None
Expand All @@ -264,11 +285,15 @@ def from_workflow_run(
workflow_run_id=workflow_run_id,
workflow_run=workflow_run,
)
if bucket_override:
_log(f" bucket_override: {bucket_override!r} (overrides {bucket!r})")
bucket = bucket_override
return cls(
bucket=bucket,
external_repo=external_repo,
run_id=run_id,
platform=platform,
path_prefix=path_prefix,
)

@classmethod
Expand Down
94 changes: 84 additions & 10 deletions build_tools/github_actions/post_build_upload.py
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This script is only used by "CI", not "Multi-arch CI", and thus is scheduled for deletion (#3340). New feature work should be in https://github.com/ROCm/TheRock/blob/main/build_tools/github_actions/post_stage_upload.py

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless NPI pipelines are changed to work with multi-arch CI, such scripts that the older CI used cannot be deleted. A discussion on how multi-arch CI can intersect NPI pipelines is overdue.

Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,23 @@
[--run-id RUN_ID]
[--output-dir OUTPUT_DIR]
[--dry-run]
[--artifacts-bucket BUCKET]
[--path-prefix PREFIX]
[--summary-base-url URL] [--summary-path PREFIX]
[--skip-manifest]

Path/URL arguments (--path-prefix, --summary-base-url, --summary-path):
--path-prefix is the S3 key prefix for all outputs; --summary-path is the
corresponding prefix in the CDN URL used for job summary links. These let
private workflows place artifacts under a versioned subdirectory while
surfacing links through a CDN that maps a different path prefix.

Example:
--path-prefix v3/artifacts
--summary-base-url https://artifacts.example.com
--summary-path artifacts
# S3 key: s3://bucket/v3/artifacts/{run_id}-linux/...
# Summary: https://artifacts.example.com/artifacts/{run_id}-linux/...

This script runs after building TheRock, where this script does:
1. Create log archives
Expand Down Expand Up @@ -200,26 +217,39 @@ def write_gha_build_summary(
build_dir: Path,
output_root: WorkflowOutputRoot,
job_status: str,
skip_manifest: bool = False,
summary_base_url: str = "",
summary_path: str = "",
):
log(f"Adding links to job summary for {output_root.prefix}")

log_index_url = output_root.log_index(artifact_group).https_url
def _url(loc) -> str:
if summary_base_url:
return loc.cdn_url(
base_url=summary_base_url,
strip_prefix=output_root.path_prefix,
cdn_prefix=summary_path,
)
return loc.https_url

log_index_url = _url(output_root.log_index(artifact_group))
gha_append_step_summary(f"[Build Logs]({log_index_url})")

observability_path = build_dir / "logs" / "build_observability.html"
if observability_path.is_file():
analysis_url = output_root.build_observability(artifact_group).https_url
analysis_url = _url(output_root.build_observability(artifact_group))
gha_append_step_summary(f"[Build Observability]({analysis_url})")
else:
log("[INFO] Build Observability: Not generated")

# Only add artifact links if the job not failed
if not job_status or job_status == "success":
artifact_url = output_root.artifact_index(artifact_group).https_url
artifact_url = _url(output_root.artifact_index(artifact_group))
gha_append_step_summary(f"[Artifacts]({artifact_url})")

manifest_url = output_root.manifest(artifact_group).https_url
gha_append_step_summary(f"[TheRock Manifest]({manifest_url})")
if not skip_manifest:
manifest_url = _url(output_root.manifest(artifact_group))
gha_append_step_summary(f"[TheRock Manifest]({manifest_url})")


def run(args):
Expand All @@ -235,7 +265,10 @@ def run(args):
return

output_root = WorkflowOutputRoot.from_workflow_run(
run_id=args.run_id, platform=PLATFORM
run_id=args.run_id,
platform=PLATFORM,
bucket_override=args.artifacts_bucket or None,
path_prefix=args.path_prefix,
)
backend = create_storage_backend(staging_dir=args.output_dir, dry_run=args.dry_run)

Expand All @@ -249,14 +282,21 @@ def run(args):
log("----------")
upload_logs(args.artifact_group, args.build_dir, output_root, backend)

log("Upload manifest")
log("----------------")
upload_manifest(args.artifact_group, args.build_dir, output_root, backend)
if not args.skip_manifest:
log("Upload manifest")
log("----------------")
upload_manifest(args.artifact_group, args.build_dir, output_root, backend)

log("Write github actions build summary")
log("--------------------")
write_gha_build_summary(
args.artifact_group, args.build_dir, output_root, args.job_status
args.artifact_group,
args.build_dir,
output_root,
args.job_status,
skip_manifest=args.skip_manifest,
summary_base_url=args.summary_base_url,
summary_path=args.summary_path,
)


Expand Down Expand Up @@ -297,10 +337,44 @@ def run(args):
action="store_true",
help="Print what would be uploaded without actually uploading",
)
parser.add_argument(
"--artifacts-bucket",
type=str,
default="",
help="Override the S3 bucket for artifacts. If unset, the bucket is determined from repository/fork/release_type. Bucket permissions are enforced by AWS IAM.",
)
parser.add_argument(
"--path-prefix",
type=str,
default="",
help="S3 key prefix for all outputs (e.g. 'v3/artifacts').",
)
parser.add_argument(
"--summary-base-url",
type=str,
default="",
help="CDN base URL for job summary links (e.g. 'https://artifacts.example.com'). When set, summary links use this instead of the raw S3 URL.",
)
parser.add_argument(
"--summary-path",
type=str,
default="",
help="CDN path prefix for summary links, corresponding to --path-prefix in storage (e.g. 'artifacts').",
)
Comment on lines +358 to +363
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Either strip trailing / characters or assert with an argument parsing error. Comments are not error handling.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Trailing / characters are now rstrip'd.

parser.add_argument(
"--skip-manifest",
action=argparse.BooleanOptionalAction,
default=False,
help="Skip manifest upload and omit the manifest link from the job summary.",
)
args = parser.parse_args()

# Check preconditions for provided arguments before proceeding.

args.path_prefix = args.path_prefix.rstrip("/")
args.summary_base_url = args.summary_base_url.rstrip("/")
args.summary_path = args.summary_path.rstrip("/")

if args.upload:
if not args.run_id:
parser.error("when --upload is true, --run_id must also be set")
Expand Down
97 changes: 97 additions & 0 deletions build_tools/github_actions/tests/post_build_upload_test.py
Original file line number Diff line number Diff line change
Expand Up @@ -28,12 +28,14 @@ def _make_output_root(
platform="linux",
bucket="therock-ci-artifacts",
external_repo="",
path_prefix="",
):
return WorkflowOutputRoot(
bucket=bucket,
external_repo=external_repo,
run_id=run_id,
platform=platform,
path_prefix=path_prefix,
)


Expand Down Expand Up @@ -363,5 +365,100 @@ def test_summary_with_external_repo(self, mock_summary):
self.assertIn("Fork-TheRock/12345-linux", call)


class TestWriteGhaBuildSummarySkipManifest(unittest.TestCase):
"""Tests for --skip-manifest flag in write_gha_build_summary."""

@mock.patch("post_build_upload.gha_append_step_summary")
def test_skip_manifest_omits_summary_link(self, mock_summary):
"""--skip-manifest suppresses the manifest link in the job summary."""
output_root = _make_output_root()
with tempfile.TemporaryDirectory() as tmp:
build_dir = Path(tmp)
post_build_upload.write_gha_build_summary(
"gfx94X-dcgpu", build_dir, output_root, "success", skip_manifest=True
)

calls = [c[0][0] for c in mock_summary.call_args_list]
self.assertEqual(len(calls), 2) # logs + artifacts only
for call in calls:
self.assertNotIn("manifest", call.lower())

@mock.patch("post_build_upload.gha_append_step_summary")
def test_manifest_present_by_default(self, mock_summary):
"""Without --skip-manifest the manifest link is included."""
output_root = _make_output_root()
with tempfile.TemporaryDirectory() as tmp:
build_dir = Path(tmp)
post_build_upload.write_gha_build_summary(
"gfx94X-dcgpu", build_dir, output_root, "success"
)

calls = [c[0][0] for c in mock_summary.call_args_list]
self.assertEqual(len(calls), 3) # logs + artifacts + manifest
self.assertTrue(any("manifest" in c.lower() for c in calls))


class TestWriteGhaBuildSummaryCdnUrl(unittest.TestCase):
"""Tests for CDN URL substitution in write_gha_build_summary."""

@mock.patch("post_build_upload.gha_append_step_summary")
def test_cdn_url_replaces_s3_url(self, mock_summary):
"""When summary_base_url is set, all links use the CDN URL."""
output_root = _make_output_root()
with tempfile.TemporaryDirectory() as tmp:
build_dir = Path(tmp)
post_build_upload.write_gha_build_summary(
"gfx94X-dcgpu",
build_dir,
output_root,
"success",
summary_base_url="https://artifacts.example.com",
)

calls = [c[0][0] for c in mock_summary.call_args_list]
for call in calls:
self.assertIn("https://artifacts.example.com", call)
self.assertNotIn("s3.amazonaws.com", call)

@mock.patch("post_build_upload.gha_append_step_summary")
def test_cdn_url_with_path_prefix_remapping(self, mock_summary):
"""strip_prefix is stripped and cdn_prefix prepended in summary URLs."""
output_root = _make_output_root(path_prefix="v3/artifacts")
with tempfile.TemporaryDirectory() as tmp:
build_dir = Path(tmp)
post_build_upload.write_gha_build_summary(
"gfx94X-dcgpu",
build_dir,
output_root,
"success",
summary_base_url="https://artifacts.example.com",
summary_path="artifacts",
)

calls = [c[0][0] for c in mock_summary.call_args_list]
# Log index link should use the CDN path prefix, not the storage path prefix
self.assertIn(
"https://artifacts.example.com/artifacts/12345-linux/logs/gfx94X-dcgpu/index.html",
calls[0],
)
for call in calls:
self.assertNotIn("v3/artifacts", call)
self.assertNotIn("s3.amazonaws.com", call)

@mock.patch("post_build_upload.gha_append_step_summary")
def test_no_summary_base_url_uses_s3(self, mock_summary):
"""Without summary_base_url, raw S3 URLs are used."""
output_root = _make_output_root()
with tempfile.TemporaryDirectory() as tmp:
build_dir = Path(tmp)
post_build_upload.write_gha_build_summary(
"gfx94X-dcgpu", build_dir, output_root, "success"
)

calls = [c[0][0] for c in mock_summary.call_args_list]
for call in calls:
self.assertIn("s3.amazonaws.com", call)


if __name__ == "__main__":
unittest.main()
Loading
Loading