From f70f7157b43c2a59a554e5f4dfc2e5d9f07bf492 Mon Sep 17 00:00:00 2001 From: Jacob Floyd Date: Thu, 2 Jan 2025 16:36:07 -0600 Subject: [PATCH] nfpm(bugfix): handle package targets w/ output_path (#21806) 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. In debugging this, I ended up copying the plugin code into a separate repo so I could figure out what was going wrong. It would have been easier to debug if the sandbox contents were listed in the error message that claims the src file was missing. So, this also expands the error messages so future issues can be identified (hopefully) more simply. --- src/python/pants/backend/nfpm/field_sets.py | 7 ++ .../nfpm/util_rules/generate_config.py | 10 ++- .../nfpm/util_rules/generate_config_test.py | 78 +++++++++++++++++++ .../backend/nfpm/util_rules/sandbox_test.py | 44 ++++++++++- 4 files changed, 132 insertions(+), 7 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.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" 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..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 @@ -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"], + {}, + ["dummy", "output_path/package"], + {}, + None, + id="apk-with-pkg-dep", + ), + pytest.param( + "archlinux", + NfpmArchlinuxPackageFieldSet, + ["contents:dummy_package"], + {}, + ["dummy", "output_path/package"], + {}, + None, + id="archlinux-with-pkg-dep", + ), + pytest.param( + "deb", + NfpmDebPackageFieldSet, + ["contents:dummy_package"], + {}, + ["dummy", "output_path/package"], + {}, + None, + id="deb-with-pkg-dep", + ), + pytest.param( + "rpm", + NfpmRpmPackageFieldSet, + ["contents:dummy_package"], + {}, + ["dummy", "output_path/package"], + {}, + None, + id="rpm-with-pkg-dep", + ), + pytest.param( + "rpm", + NfpmRpmPackageFieldSet, + ["contents:dummy_package"], + {}, + ["dummy", "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,25 @@ def test_generate_nfpm_yaml( dst="/etc/{_PKG_NAME}", file_mode=0o700, ) + 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" + # 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="output_path/package", + dst="/usr/bin/dummy_package", + file_mode=0o550, + ) nfpm_content_file( name="malformed", # needs either source or src. dst="/usr/bin/foo", @@ -630,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 de08effdce6..2a4d41e5dd5 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=[":output_path_archive"], + ) """ ), "package/archive-contents.txt": "",