From 59d013c63d6baea9ebe26e92015cb6cfe314087f Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 2 Jan 2025 12:57:43 -0600 Subject: [PATCH 1/3] nfpm(bugfix): handle package targets w/ output_path A package target with an output_path puts the packaged file relative to the build_root instead of relative to the BUILD file. So, we need to explicitly check for both the file_path (relative to BUILD file) and the value (which might be relative to the build_root) instead of just the file_path. --- src/python/pants/backend/nfpm/field_sets.py | 7 ++ .../nfpm/util_rules/generate_config_test.py | 69 +++++++++++++++++++ .../backend/nfpm/util_rules/sandbox_test.py | 44 ++++++++++-- 3 files changed, 116 insertions(+), 4 deletions(-) diff --git a/src/python/pants/backend/nfpm/field_sets.py b/src/python/pants/backend/nfpm/field_sets.py index 77b22453908..f7d2e7af708 100644 --- a/src/python/pants/backend/nfpm/field_sets.py +++ b/src/python/pants/backend/nfpm/field_sets.py @@ -238,12 +238,19 @@ def nfpm_config( ) -> NfpmContent: source: str | None = self.source.file_path src: str | None = self.src.file_path + src_value: str | None = self.src.value dst: str = self.dst.value if source is not None and not src: # If defined, 'source' provides the default value for 'src'. src = source + src_value = self.source.value if src is None: # src is NOT required; prepare to raise an error. raise self.InvalidTarget() + if src not in content_sandbox_files and src_value in content_sandbox_files: + # A field's file_path assumes the field's value is relative to the BUILD file. + # But, for packages the field's value can be relative to the build_root instead, + # because a package's output_path can be any arbitrary build_root relative path. + src = src_value sandbox_file: FileEntry | None = content_sandbox_files.get(src) if sandbox_file is None: raise self.SrcMissingFomSandbox() diff --git a/src/python/pants/backend/nfpm/util_rules/generate_config_test.py b/src/python/pants/backend/nfpm/util_rules/generate_config_test.py index 909673f10ba..a59e62f3237 100644 --- a/src/python/pants/backend/nfpm/util_rules/generate_config_test.py +++ b/src/python/pants/backend/nfpm/util_rules/generate_config_test.py @@ -261,6 +261,57 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: None, id="rpm-with-deps-and-ghost", ), + # with dummy_package dependency + pytest.param( + "apk", + NfpmApkPackageFieldSet, + ["contents:dummy_package"], + {}, + ["output_path/package"], + {}, + None, + id="apk-with-pkg-dep", + ), + pytest.param( + "archlinux", + NfpmArchlinuxPackageFieldSet, + ["contents:dummy_package"], + {}, + ["output_path/package"], + {}, + None, + id="archlinux-with-pkg-dep", + ), + pytest.param( + "deb", + NfpmDebPackageFieldSet, + ["contents:dummy_package"], + {}, + ["output_path/package"], + {}, + None, + id="deb-with-pkg-dep", + ), + pytest.param( + "rpm", + NfpmRpmPackageFieldSet, + ["contents:dummy_package"], + {}, + ["output_path/package"], + {}, + None, + id="rpm-with-pkg-dep", + ), + pytest.param( + "rpm", + NfpmRpmPackageFieldSet, + ["contents:dummy_package"], + {}, + ["output_path/package"], + {"ghost_contents": ["/var/log/pkg.log"]}, + None, + id="rpm-with-pkg-dep-and-ghost", + ), # with malformed dependency pytest.param( "apk", @@ -477,6 +528,24 @@ def test_generate_nfpm_yaml( dst="/etc/{_PKG_NAME}", file_mode=0o700, ) + target( + name="dummy", + # For this test, the dummy target should be + # treated as if it were a package that defines: + # output_path="output_path/package" + # Instead of wiring an actual package target, + # the test imitates packaging by injecting the file + # into the digest. + ) + nfpm_content_file( + name="dummy_package", + dependencies=[":dummy"], + # NOTE: src pointing to package dep is relative to + # the build_root instead of this BUILD file. + src="ouptut_path/package", + dst="/usr/bin/dummy_package", + file_mode=0o550, + ) nfpm_content_file( name="malformed", # needs either source or src. dst="/usr/bin/foo", diff --git a/src/python/pants/backend/nfpm/util_rules/sandbox_test.py b/src/python/pants/backend/nfpm/util_rules/sandbox_test.py index de08effdce6..d96d9f8433e 100644 --- a/src/python/pants/backend/nfpm/util_rules/sandbox_test.py +++ b/src/python/pants/backend/nfpm/util_rules/sandbox_test.py @@ -326,48 +326,72 @@ def rule_runner() -> RuleRunner: pytest.param( "apk", NfpmApkPackageFieldSet, - ["codegen:generated", "contents:files", "package:package"], + [ + "codegen:generated", + "contents:files", + "package:package", + "package:output_path_package", + ], {}, { "codegen/foobar.codegen.generated", "contents/sandbox-file.txt", "package/archive.tar", + "relative_to_build_root.tar", }, id="apk-codegen-and-package", ), pytest.param( "archlinux", NfpmArchlinuxPackageFieldSet, - ["codegen:generated", "contents:files", "package:package"], + [ + "codegen:generated", + "contents:files", + "package:package", + "package:output_path_package", + ], {}, { "codegen/foobar.codegen.generated", "contents/sandbox-file.txt", "package/archive.tar", + "relative_to_build_root.tar", }, id="archlinux-codegen-and-package", ), pytest.param( "deb", NfpmDebPackageFieldSet, - ["codegen:generated", "contents:files", "package:package"], + [ + "codegen:generated", + "contents:files", + "package:package", + "package:output_path_package", + ], {}, { "codegen/foobar.codegen.generated", "contents/sandbox-file.txt", "package/archive.tar", + "relative_to_build_root.tar", }, id="deb-codegen-and-package", ), pytest.param( "rpm", NfpmRpmPackageFieldSet, - ["codegen:generated", "contents:files", "package:package"], + [ + "codegen:generated", + "contents:files", + "package:package", + "package:output_path_package", + ], {}, { "codegen/foobar.codegen.generated", "contents/sandbox-file.txt", "package/archive.tar", + "relative_to_build_root.tar", }, id="rpm-codegen-and-package", ), @@ -459,6 +483,18 @@ def test_populate_nfpm_content_sandbox( dst="/opt/foo/archive.tar", dependencies=[":archive"], ) + archive( + name="output_path_archive", + format="tar", + output_path="relative_to_build_root.tar", + files=[":file"], + ) + nfpm_content_file( + name="output_path_package", + src="relative_to_build_root.tar", + dst="/opt/foo/relative_to_build_root.tar", + dependencies=[":archive"], + ) """ ), "package/archive-contents.txt": "", From 755cde1172bd7f997e155b04b31f80cf81ef60c1 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 2 Jan 2025 13:26:56 -0600 Subject: [PATCH 2/3] nfpm: Improve sandbox error messages Debugging missing files is very difficult because the sandbox cannot be inspected until pants creates a process sandbox. The src is missing error happens before running nfpm, however, so there is no sandbox to "preserve on_failure". So, the error message also includes the list of sandbox files now. --- .../pants/backend/nfpm/util_rules/generate_config.py | 10 +++++++--- 1 file changed, 7 insertions(+), 3 deletions(-) diff --git a/src/python/pants/backend/nfpm/util_rules/generate_config.py b/src/python/pants/backend/nfpm/util_rules/generate_config.py index 9ec49bfb856..afbca3560ad 100644 --- a/src/python/pants/backend/nfpm/util_rules/generate_config.py +++ b/src/python/pants/backend/nfpm/util_rules/generate_config.py @@ -117,8 +117,8 @@ async def generate_nfpm_yaml( defaults to the file referenced in the '{NfpmContentFileSourceField.alias}' field. Please fix the {'targets at these addresses' if plural else 'target at this address'}: """ - + ",\n".join(str(address) for address in invalid_content_file_addresses) ) + + "\n".join(str(address) for address in invalid_content_file_addresses) ) if src_missing_from_sandbox_addresses: plural = len(src_missing_from_sandbox_addresses) > 1 @@ -129,10 +129,12 @@ async def generate_nfpm_yaml( from the nfpm sandbox. This sandbox contains packages, generated code, and sources from the '{NfpmDependencies.alias}' field. It also contains any file from the '{NfpmContentFileSourceField.alias}' field. Please fix the '{NfpmContentFile.alias}' - {'targets at these addresses' if plural else 'target at this address'}.: + {'targets at these addresses' if plural else 'target at this address'}: """ - + ",\n".join(str(address) for address in src_missing_from_sandbox_addresses) ) + + "\n".join(str(address) for address in src_missing_from_sandbox_addresses) + + "\n\nThe sandbox contains these files:\n" + + "\n".join(str(path) for path in content_sandbox_files) ) contents.sort(key=lambda d: d["dst"]) @@ -155,6 +157,8 @@ async def generate_nfpm_yaml( {repr(script_src_missing_from_sandbox)} """ ) + + "\n\nThe sandbox contains these files:\n" + + "\n".join(str(path) for path in content_sandbox_files) ) nfpm_yaml = "# Generated by Pantsbuild\n" From 381ceb82e79fa9051a1f566310d418bbc8ce1b26 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 2 Jan 2025 14:08:38 -0600 Subject: [PATCH 3/3] nfpm: fix util_rule tests --- .../nfpm/util_rules/generate_config_test.py | 23 +++++++++++++------ .../backend/nfpm/util_rules/sandbox_test.py | 2 +- 2 files changed, 17 insertions(+), 8 deletions(-) diff --git a/src/python/pants/backend/nfpm/util_rules/generate_config_test.py b/src/python/pants/backend/nfpm/util_rules/generate_config_test.py index a59e62f3237..d5f9a8863a8 100644 --- a/src/python/pants/backend/nfpm/util_rules/generate_config_test.py +++ b/src/python/pants/backend/nfpm/util_rules/generate_config_test.py @@ -267,7 +267,7 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: NfpmApkPackageFieldSet, ["contents:dummy_package"], {}, - ["output_path/package"], + ["dummy", "output_path/package"], {}, None, id="apk-with-pkg-dep", @@ -277,7 +277,7 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: NfpmArchlinuxPackageFieldSet, ["contents:dummy_package"], {}, - ["output_path/package"], + ["dummy", "output_path/package"], {}, None, id="archlinux-with-pkg-dep", @@ -287,7 +287,7 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: NfpmDebPackageFieldSet, ["contents:dummy_package"], {}, - ["output_path/package"], + ["dummy", "output_path/package"], {}, None, id="deb-with-pkg-dep", @@ -297,7 +297,7 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: NfpmRpmPackageFieldSet, ["contents:dummy_package"], {}, - ["output_path/package"], + ["dummy", "output_path/package"], {}, None, id="rpm-with-pkg-dep", @@ -307,7 +307,7 @@ def get_digest(rule_runner: RuleRunner, source_files: dict[str, str]) -> Digest: NfpmRpmPackageFieldSet, ["contents:dummy_package"], {}, - ["output_path/package"], + ["dummy", "output_path/package"], {"ghost_contents": ["/var/log/pkg.log"]}, None, id="rpm-with-pkg-dep-and-ghost", @@ -528,8 +528,9 @@ def test_generate_nfpm_yaml( dst="/etc/{_PKG_NAME}", file_mode=0o700, ) - target( + file( name="dummy", + source="dummy", # For this test, the dummy target should be # treated as if it were a package that defines: # output_path="output_path/package" @@ -542,7 +543,7 @@ def test_generate_nfpm_yaml( dependencies=[":dummy"], # NOTE: src pointing to package dep is relative to # the build_root instead of this BUILD file. - src="ouptut_path/package", + src="output_path/package", dst="/usr/bin/dummy_package", file_mode=0o550, ) @@ -699,6 +700,14 @@ def test_generate_nfpm_yaml( entry = contents_by_dst.pop(dst) assert "ghost" == entry["type"] + if "contents:dummy_package" in dependencies: + assert "dummy" not in contents_by_dst + dst = "/usr/bin/dummy_package" + assert dst in contents_by_dst + entry = contents_by_dst.pop(dst) + assert "" == entry["type"] + assert 0o0550 == entry["file_info"]["mode"] + # make sure all contents have been accounted for (popped off above) assert len(contents_by_dst) == 0 diff --git a/src/python/pants/backend/nfpm/util_rules/sandbox_test.py b/src/python/pants/backend/nfpm/util_rules/sandbox_test.py index d96d9f8433e..2a4d41e5dd5 100644 --- a/src/python/pants/backend/nfpm/util_rules/sandbox_test.py +++ b/src/python/pants/backend/nfpm/util_rules/sandbox_test.py @@ -493,7 +493,7 @@ def test_populate_nfpm_content_sandbox( name="output_path_package", src="relative_to_build_root.tar", dst="/opt/foo/relative_to_build_root.tar", - dependencies=[":archive"], + dependencies=[":output_path_archive"], ) """ ),