update-build-files: add support for Buildifier formatter #21208

AlexTereshenkov · 2024-07-25T21:09:30Z

buildifier was added to the list of supported formatters that can be used to format the BUILD files with the update-build-files goal.

It may be helpful if your organization is migrating from Bazel and wants to keep the style of the BUILD files
consistent or if for any other reason you may want to adopt the formatting style that is enforced by buildifier.

The addition of the Buildifier support was added in https://github.com/pantsbuild/pants/pull/16573/files; this PR adds a relevant option to actually let users choose it to format their BUILD files.

The buildifier can be used on its own, but it can also be used in pair with a Python formatter, such as black
or ruff. For instance, you could first run buildifier to sort the target fields alphabetically,
and then run black to keep the style consistent with the rest of the Python code.

E.g. running

$ pants update-build-files --fmt --formatter=buildifier src/python/pants/backend/python/util_rules/BUILD
$ pants update-build-files --fmt --formatter=black src/python/pants/backend/python/util_rules/BUILD

would convert

resource(name="complete_platform_pex_test", source="complete_platform_pex_test.json")

python_sources(
    overrides={
        "local_dists_pep660.py": dict(dependencies=["./scripts/pep660_backend_wrapper.py"]),
        },
    name="foo",
)

to

# adding a trailing comma, putting one argument per line
resource(
    name="complete_platform_pex_test",
    source="complete_platform_pex_test.json",
)

# reorder the fields, putting the name first
python_sources(
    name="foo",
    overrides={
        "local_dists_pep660.py": dict(dependencies=["./scripts/pep660_backend_wrapper.py"]),
    },
)

huonw

Thanks for this.

I'm a little bit confused. It looks like the existing src/python/pants/backend/build_files/fmt/buildifier/rules.py code is focused on formatting build files, e.g. its request is called Fmt BuildFiles Request (emphasis mine). That suggests to me that there should already be a way to format build files with buildifier... but I'm probably missing something? Any insight?

huonw · 2024-07-25T22:30:39Z

src/python/pants/backend/build_files/fmt/buildifier/rules.py

+# Note - this function is kept separate because it is invoked from update_build_files.py, but
+# not as a rule.


Could we either invoke it as a rule, or have buildfier_fmt call this function to avoid the duplication?

Yeah, I think having buildifier_fmt call this function is probably best - this is how the ruff backend works too:

https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/build_files/fmt/ruff/register.py#L19-L22

https://github.com/pantsbuild/pants/blob/main/src/python/pants/backend/python/lint/ruff/format/rules.py#L69-L71

Oh of course, this has been now refactored.

And now I recalled why it was duplicate - there seems to be an issue to have a call to a function which invokes other rules:

E native_engine.IntrinsicError: Call(pants.core.util_rules.external_tool.download_external_tool, DownloadedExternalTool) was not detected in your @rule body at rule compile time.

This must a Pants architecture constraint. I'll poke around a bit more.

Leaving the duplicate code as is for now.

Gotcha - I think this may be an issue with the new call-by-name syntax rather than the prior version of the engine. Good to know on a go-forward basis, and I think we'll need to take this into account for the other build files formatters.

Huh, okay.

Happy to land this as is to keep moving forward, but it'd be good to have a follow-up that resolves the duplication... potentially even just deleting the @rule entirely, since it's apparently not used for anything real? (as evidenced by the backend not being wired up/not doing anything)

That said, I believe it is be normal and supported for @rules to call non-@rule async def functions, so if call-by-name doesn't support it, we should know about it and probably fix it, e.g. random example await _get_relevant_source_files:

pants/src/python/pants/backend/javascript/install_node_package.py

Lines 65 to 88 in 459d39b

async def _get_relevant_source_files(

sources: Iterable[SourcesField], with_js: bool = False

) -> SourceFiles:

return await Get(

SourceFiles,

SourceFilesRequest(

sources,

for_sources_types=(PackageJsonSourceField, FileSourceField)

+ ((ResourceSourceField, JSSourceField) if with_js else ()),

enable_codegen=True,

),

)

@rule

async def install_node_packages_for_address(

req: InstalledNodePackageRequest, union_membership: UnionMembership

) -> InstalledNodePackage:

project_env = await Get(NodeJsProjectEnvironment, NodeJSProjectEnvironmentRequest(req.address))

target = project_env.ensure_target()

transitive_tgts = await Get(TransitiveTargets, TransitiveTargetsRequest([target.address]))

source_files = await _get_relevant_source_files(

As part of the follow-up can you investigate more deeply and maybe file a bug?

Thanks, Huon. Definitely, let me either do a follow-up PR with either a clean up or refactoring or filing an issue.

krishnan-chandra

Looks good minus Huon's comment - I think what was missing here before was wiring the existing backend up within goals/update_build_files.py; while the backend was created in #16573, it doesn't appear that the Buildifier rules were added to the goal.

… duplication

AlexTereshenkov · 2024-07-26T07:48:06Z

That suggests to me that there should already be a way to format build files with buildifier... but I'm probably missing something? Any insight?

@huonw It appears the formatter was added, but not wired. This might have been intentional because it's a bit of a special usage as Buildifier is intended to be used on Starlark, not on Pants BUILD files which may have unsupported syntax.

krishnan-chandra · 2024-07-26T13:11:31Z

src/python/pants/backend/build_files/fmt/buildifier/rules.py

 # Note - this function is kept separate because it is invoked from update_build_files.py, but
-# not as a rule.
+# not as a rule. This function cannot be called from the `buildifier_fmt` rule because other rules


If this is true, then _run_buildifier_fmt is only used in tests? I think we'll also probably have to figure out another way to run the rules in other formatter backends as well since they follow the same pattern.

Is it possible to invoke the buildifier_fmt rule in the tests directly without needing this helper function?

Nope, it's used in the sources, actually:

pants/src/python/pants/core/goals/update_build_files.py

Line 440 in a512943

result = await _run_buildifier_fmt(

I see. Is it possible to make that call invoke the buildifier_fmt rule instead of the helper function, or does that not work either?

If the call-by-name architecture constraint is that rules should not be invoked from non-rule functions, then we should keep the chain consistent and call the right rule from format_build_file_with_buildifier.

Nope, can't call buildifier_fmt rule from the format_build_file_with_buildifier rule.

File "/home/username/code/pants/src/python/pants/engine/internals/rule_visitor.py", line 277, in _get_byname_awaitable raise parse_error( pants.engine.internals.selectors.GetParseError: Invalid Get. /home/username/code/pants/src/python/pants/core/goals/update_build_files.py:440: Expected an `**implicitly(..)` application as the only keyword input. Failed for Get() in /home/username/code/pants/src/python/pants/core/goals/update_build_files.py.

AlexTereshenkov · 2024-08-06T16:24:53Z

@huonw I am sorry I am not sure if you have any feedback for me since the last interaction we had in this PR? No rush, just checking if I have missed something.

huonw

Looks good, other than:

the note about follow up
can you update the PR description before merging to reference Add buildifier as a BUILD file formatter #16573 and how this is doing the wiring up that was missed there?

huonw · 2024-08-07T01:53:11Z

src/python/pants/core/goals/update_build_files_test.py

+# ------------------------------------------------------------------------------------------
+
+
+def run_buildifier(


Good to have a test 👍

huonw · 2024-08-07T01:57:19Z

src/python/pants/backend/build_files/fmt/buildifier/rules.py

+# Note - this function is kept separate because it is invoked from update_build_files.py, but
+# not as a rule.


Huh, okay.

Happy to land this as is to keep moving forward, but it'd be good to have a follow-up that resolves the duplication... potentially even just deleting the @rule entirely, since it's apparently not used for anything real? (as evidenced by the backend not being wired up/not doing anything)

That said, I believe it is be normal and supported for @rules to call non-@rule async def functions, so if call-by-name doesn't support it, we should know about it and probably fix it, e.g. random example await _get_relevant_source_files:

pants/src/python/pants/backend/javascript/install_node_package.py

Lines 65 to 88 in 459d39b

async def _get_relevant_source_files(

sources: Iterable[SourcesField], with_js: bool = False

) -> SourceFiles:

return await Get(

SourceFiles,

SourceFilesRequest(

sources,

for_sources_types=(PackageJsonSourceField, FileSourceField)

+ ((ResourceSourceField, JSSourceField) if with_js else ()),

enable_codegen=True,

),

)

@rule

async def install_node_packages_for_address(

req: InstalledNodePackageRequest, union_membership: UnionMembership

) -> InstalledNodePackage:

project_env = await Get(NodeJsProjectEnvironment, NodeJSProjectEnvironmentRequest(req.address))

target = project_env.ensure_target()

transitive_tgts = await Get(TransitiveTargets, TransitiveTargetsRequest([target.address]))

source_files = await _get_relevant_source_files(

As part of the follow-up can you investigate more deeply and maybe file a bug?

update-build-files: add support for Buildifier formatter

a512943

AlexTereshenkov added the category:new feature label Jul 25, 2024

AlexTereshenkov requested review from huonw, kaos and krishnan-chandra July 25, 2024 21:59

AlexTereshenkov marked this pull request as ready for review July 25, 2024 21:59

huonw reviewed Jul 25, 2024

View reviewed changes

krishnan-chandra reviewed Jul 25, 2024

View reviewed changes

update-build-files: make rule call an internal function to avoid code…

fa8d1e0

… duplication

AlexTereshenkov requested review from huonw and krishnan-chandra July 26, 2024 07:48

update-build-files: keep duplicate code for rule invocation

4aea83c

krishnan-chandra reviewed Jul 26, 2024

View reviewed changes

huonw approved these changes Aug 7, 2024

View reviewed changes

AlexTereshenkov merged commit 136eadd into pantsbuild:main Aug 9, 2024
25 checks passed

AlexTereshenkov deleted the pants/add-buildifer-as-build-file-formatter branch August 9, 2024 12:16

huonw mentioned this pull request Aug 12, 2024

update-build-files: bump buildifier version #21289

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

update-build-files: add support for Buildifier formatter #21208

update-build-files: add support for Buildifier formatter #21208

AlexTereshenkov commented Jul 25, 2024 •

edited

Loading

huonw left a comment

huonw Jul 25, 2024

krishnan-chandra Jul 25, 2024

AlexTereshenkov Jul 26, 2024

AlexTereshenkov Jul 26, 2024 •

edited

Loading

krishnan-chandra Jul 26, 2024

huonw Aug 7, 2024 •

edited

Loading

AlexTereshenkov Aug 9, 2024

krishnan-chandra left a comment •

edited

Loading

AlexTereshenkov commented Jul 26, 2024

krishnan-chandra Jul 26, 2024

AlexTereshenkov Jul 26, 2024

krishnan-chandra Jul 26, 2024 •

edited

Loading

AlexTereshenkov Jul 26, 2024 •

edited

Loading

AlexTereshenkov commented Aug 6, 2024

huonw left a comment

huonw Aug 7, 2024

huonw Aug 7, 2024 •

edited

Loading

		# Note - this function is kept separate because it is invoked from update_build_files.py, but
		# not as a rule.


	async def _get_relevant_source_files(
	sources: Iterable[SourcesField], with_js: bool = False
	) -> SourceFiles:
	return await Get(
	SourceFiles,
	SourceFilesRequest(
	sources,
	for_sources_types=(PackageJsonSourceField, FileSourceField)
	+ ((ResourceSourceField, JSSourceField) if with_js else ()),
	enable_codegen=True,
	),
	)


	@rule
	async def install_node_packages_for_address(
	req: InstalledNodePackageRequest, union_membership: UnionMembership
	) -> InstalledNodePackage:
	project_env = await Get(NodeJsProjectEnvironment, NodeJSProjectEnvironmentRequest(req.address))
	target = project_env.ensure_target()
	transitive_tgts = await Get(TransitiveTargets, TransitiveTargetsRequest([target.address]))

	source_files = await _get_relevant_source_files(

		# ------------------------------------------------------------------------------------------


		def run_buildifier(

update-build-files: add support for Buildifier formatter #21208

update-build-files: add support for Buildifier formatter #21208

Conversation

AlexTereshenkov commented Jul 25, 2024 • edited Loading

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexTereshenkov Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnan-chandra left a comment • edited Loading

Choose a reason for hiding this comment

AlexTereshenkov commented Jul 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

krishnan-chandra Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

AlexTereshenkov Jul 26, 2024 • edited Loading

Choose a reason for hiding this comment

AlexTereshenkov commented Aug 6, 2024

huonw left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

huonw Aug 7, 2024 • edited Loading

Choose a reason for hiding this comment

AlexTereshenkov commented Jul 25, 2024 •

edited

Loading

AlexTereshenkov Jul 26, 2024 •

edited

Loading

huonw Aug 7, 2024 •

edited

Loading

krishnan-chandra left a comment •

edited

Loading

krishnan-chandra Jul 26, 2024 •

edited

Loading

AlexTereshenkov Jul 26, 2024 •

edited

Loading

huonw Aug 7, 2024 •

edited

Loading