Skip to content

Bug: incorrect printout how to fix formatting issues #138

Closed
Closed
Bug: incorrect printout how to fix formatting issues#138
Assignees
nicu1989
Labels
community:infrastructureGeneral Score infrastructure topicsgood first issueGood for newcomers

Description

@AlexanderLanin
AlexanderLanin
opened on Jan 8, 2025

When a yaml is formatted incorrectly (e.g. github workflow) the error message is:

FAILED: A formatter tool exited with code 123
Try running 'bazel run //tools/format:format.check_YAML_with_yamlfmt ' to fix this.

However, running said command just produces the output above.
The correct command is: bazel run //tools/format:format.fix_YAML_with_yamlfmt.

Same for python: bazel run //tools/format:format.check_Python_with_ruff does not "fix" the formatting issues.

The check must report the correct command which is needed to fix formatting issues.

Full example:

bazel run //tools/format:format.check_Python_with_ruff
INFO: Analyzed target //tools/format:format.check_Python_with_ruff (5 packages loaded, 26 targets configured).
INFO: Found 1 target...
Target //tools/format:format.check_Python_with_ruff up-to-date:
  bazel-bin/tools/format/format.check_Python_with_ruff
INFO: Elapsed time: 1.090s, Critical Path: 0.15s
INFO: 6 processes: 6 internal.
INFO: Build completed successfully, 6 total actions
INFO: Running command line: external/bazel_tools/tools/test/test-setup.sh tools/format/format.check_Python_with_ruff
exec ${PAGER:-/usr/bin/less} "$0" || exit 1
Executing tests from //tools/format:format.check_Python_with_ruff
-----------------------------------------------------------------------------
--- docs/_tooling/incremental.py
+++ docs/_tooling/incremental.py
@@ -24,9 +24,11 @@
 
 sphinx_main(
     [
-        "docs", # src dir
-        "_build", # out dir
-        "--jobs", "auto",
-        "--conf-dir", "docs",
+        "docs",  # src dir
+        "_build",  # out dir
+        "--jobs",
+        "auto",
+        "--conf-dir",
+        "docs",
     ]
 )

1 file would be reformatted, 4 files already formatted
Formatted Python in 0m0.156s
FAILED: A formatter tool exited with code 123
Try running 'bazel run //tools/format:format.check_Python_with_ruff ' to fix this.

Activity

  1. AlexanderLanin
    changed the title [-]Bug: yaml formatting command[/-] [+]Bug: incorrect printout how to fix formatting issues[/+] on Jan 9, 2025

  2. MaximilianSoerenPollak commented on Jan 10, 2025

    @MaximilianSoerenPollak
    MaximilianSoerenPollak
    on Jan 10, 2025
    Contributor

    Should the fix command not just be bazel run //:format.fix ?

  3. AlexanderLanin
    added
    community:infrastructureGeneral Score infrastructure topics
    on Jan 21, 2025
  4. AlexanderLanin
    moved this to Todo in INF - Infrastructure Communityon Jan 21, 2025
  5. AlexanderLanin
    added this to INF - Infrastructure Communityon Jan 21, 2025
  6. AlexanderLanin
    added
    good first issueGood for newcomers
    on Jan 28, 2025
  7. nicu1989
    self-assigned this
    on Feb 3, 2025
  8. nicu1989
    moved this from Todo to In Progress in INF - Infrastructure Communityon Feb 3, 2025

  9. nicu1989 commented on Feb 3, 2025

    @nicu1989
    nicu1989
    on Feb 3, 2025
    Member

    Issue Analysis

    The issue arises due to the way format_multirun and format_test macros are implemented by aspect-build.

    format_multirun produces a target named [name].check, which does not modify files but instead exits with a non-zero status if any sources require formatting.

    format_test is intended to be used with bazel test to verify that files are formatted.

    Getingt the correct suggestion output

    The following command lists all current targets:

    bazel query '//tools/format:*'

    Output

    # Current targets
//tools/format:BUILD
//tools/format:format.check
//tools/format:format.check_Python_with_ruff
//tools/format:format.check_Starlark_with_buildifier
//tools/format:format.check_YAML_with_yamlfmt
//tools/format:format.fix
//tools/format:format.fix.check
//tools/format:format.fix_Python_with_ruff
//tools/format:format.fix_Python_with_ruff.check
//tools/format:format.fix_Starlark_with_buildifier
//tools/format:format.fix_Starlark_with_buildifier.check
//tools/format:format.fix_YAML_with_yamlfmt
//tools/format:format.fix_YAML_with_yamlfmt.check

    To get the correct suggestion output, run the check command of the format.fix target:

    bazel run //tools/format:format.fix_YAML_with_yamlfmt.check

    Output

    ...
Formatted YAML in 0m0.028s
FAILED: A formatter tool exited with code 123
Try running 'bazel run //tools/format:format.fix_YAML_with_yamlfmt' to fix this.

    Proposed Improvement: Renaming Targets

    We can improve usability by renaming the targets as follows:

    tools\format\BUILD:

    format_multirun(
    name = "format",
    ...
)

format_test(
    name = "format_test",
    ...
)

    After Renaming, the targets would be:

    # Updated targets
//tools/format:BUILD
//tools/format:format
//tools/format:format.check
//tools/format:format_Python_with_ruff
//tools/format:format_Python_with_ruff.check
//tools/format:format_Starlark_with_buildifier
//tools/format:format_Starlark_with_buildifier.check
//tools/format:format_YAML_with_yamlfmt
//tools/format:format_YAML_with_yamlfmt.check
//tools/format:format_test
//tools/format:format_test_Python_with_ruff
//tools/format:format_test_Starlark_with_buildifier
//tools/format:format_test_YAML_with_yamlfmt

    Now, the following commands will be used:

    Action Command
    Check formatting for YAML bazel run //tools/format:format_YAML_with_yamlfmt.check
    Suggested fix command bazel run //tools/format:format_YAML_with_yamlfmt
    Check all languages bazel run //tools/format:format.check
    Run format tests for all languages bazel test //tools/format:format_test
    Fix formatting for all languages bazel run //tools/format:format

    Note

    • Updating target names requires corresponding updates in:
      • README.md
      • Aliases

  10. AlexanderLanin commented on Feb 5, 2025

    @AlexanderLanin
    AlexanderLanin
    on Feb 5, 2025
    MemberAuthor

    @nradakovic can you provide feedback here?

  11. ltekieli commented on Feb 7, 2025

    @ltekieli
    ltekieli
    on Feb 7, 2025
    Member

    @AlexanderLanin @nicu1989 I've took a look and I don't see how renaming targets will lead to a better command suggestion.

    Tried this:

    diff --git a/BUILD b/BUILD
index b423cbc..f405edd 100644
--- a/BUILD
+++ b/BUILD
@@ -15,12 +15,12 @@ load("//tools/cr_checker:cr_checker.bzl", "copyright_checker")
 
 test_suite(
     name = "format.check",
-    tests = ["//tools/format:format.check"],
+    tests = ["//tools/format:format_test"],
 )
 
 alias(
     name = "format.fix",
-    actual = "//tools/format:format.fix",
+    actual = "//tools/format:format",
 )
 
 copyright_checker(
diff --git a/MODULE.bazel b/MODULE.bazel
index 57d4007..61f4210 100644
--- a/MODULE.bazel
+++ b/MODULE.bazel
@@ -78,4 +78,4 @@ bazel_dep(name = "buildifier_prebuilt", version = "7.3.1")
 # Generic linting and formatting rules
 #
 ###############################################################################
-bazel_dep(name = "aspect_rules_lint", version = "1.0.3")
+bazel_dep(name = "aspect_rules_lint", version = "1.1.0")
diff --git a/tools/format/BUILD b/tools/format/BUILD
index 3bf7c6d..510ddf7 100644
--- a/tools/format/BUILD
+++ b/tools/format/BUILD
@@ -14,7 +14,7 @@
 load("@aspect_rules_lint//format:defs.bzl", "format_multirun", "format_test")
 
 format_multirun(
-    name = "format.fix",
+    name = "format",
     python = "@aspect_rules_lint//format:ruff",
     starlark = "@buildifier_prebuilt//:buildifier",
     visibility = [
@@ -24,7 +24,7 @@ format_multirun(
 )
 
 format_test(
-    name = "format.check",
+    name = "format_test",
     no_sandbox = True,
     python = "@aspect_rules_lint//format:ruff",
     starlark = "@buildifier_prebuilt//:buildifier",


    bazel test //:format.check --test_output=all
...
Formatted Python in 0m0.022s
FAILED: A formatter tool exited with code 123
Try running 'bazel run //tools/format:format_test_Python_with_ruff ' to fix this.

    The problem lies in the implementation in rules_lint. The suggestion just uses the same command as run for check: https://github.com/aspect-build/rules_lint/blob/3bf498bc3b92ac96818165a8ee989ba7ad06cdcd/format/private/format.sh#L47

  12. nicu1989 commented on Feb 12, 2025

    @nicu1989
    nicu1989
    on Feb 12, 2025
    Member

    The problem lies in the implementation in rules_lint.

    True, that's my conclusion also. There is not much what we can do about it. But by changing the target names we can address somewhat the usecase presented in the ticket(having the same outputed suggestion as the command):

    Current:

    • run: bazel run //tools/format:format.check_YAML_with_yamlfmt
    • outputed suggestion: bazel run //tools/format:format.check_YAML_with_yamlfmt <- exactly the same as the run command, does not fix the formatting issue

    Modified target names:

    • run: bazel run //tools/format:format_YAML_with_yamlfmt.check
    • outputed suggestion: bazel run //tools/format:format_YAML_with_yamlfmt <- no check in the suggestion, and it actually does what is supposed to do.
  13. AlexanderLanin
    added a parent issue on Feb 12, 2025

  14. MaximilianSoerenPollak commented on Feb 12, 2025

    @MaximilianSoerenPollak
    MaximilianSoerenPollak
    on Feb 12, 2025
    Contributor

    I think easiest would be to just have a note somewhere that one can fix any of the 'formatting' issues via the bazel run //:format.fix ?

  15. AlexanderLanin commented on Feb 13, 2025

    @AlexanderLanin
    AlexanderLanin
    on Feb 13, 2025
    MemberAuthor

    I think easiest would be to just have a note somewhere that one can fix any of the 'formatting' issues via the bazel run //:format.fix ?

    It's in README. This ticket was created because CI checks print incorrect output.

  16. AlexanderLanin commented on Feb 13, 2025

    @AlexanderLanin
    AlexanderLanin
    on Feb 13, 2025
    MemberAuthor

    @nicu1989 how about we merge this one here AND create an issue upstream?

  17. nicu1989 commented on Feb 14, 2025

    @nicu1989
    nicu1989
    on Feb 14, 2025
    Member

    I have created an issue to aspect-build/rules_lint: aspect-build/rules_lint#482

  18. nicu1989
    closed this as completedon Feb 14, 2025
  19. github-project-automation
    moved this from In Progress to Done in INF - Infrastructure Communityon Feb 14, 2025
  20. github-project-automation
    added this to S-CORE Roadmapon Mar 14, 2025
  21. markert-r
    moved this to Done in S-CORE Roadmapon Apr 9, 2025
  22. github-project-automation
    added this to Milestone Planning - Obsoleteon Jun 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Metadata

Metadata

Assignees

Labels

community:infrastructureGeneral Score infrastructure topicsgood first issueGood for newcomers

Type

No type

Projects

Milestone

No milestone

Relationships

None yet

Development

No branches or pull requests

Issue actions