Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a new target "ninja clippy" for Rust projects #13914

Open
wants to merge 9 commits into
base: master
Choose a base branch
from

Conversation

bonzini
Copy link
Contributor

@bonzini bonzini commented Nov 17, 2024

Running clippy by setting a custom rustc compiler that points to "clippy-driver" is suboptimal, because it requires reconfiguration. Similar to the "ninja scan-build" target for C, add a clippy internal tool that runs clippy-driver on all crates in the project.

This also provide infrastructure to find sibling tools to rustc, such as "clippy-driver" itself or rustdoc, when using the "rustup run TOOLCHAIN rustc" to invoke rustc. Adding rustfmt for example is trivial.

A few extra new features are included as a bonus 👼

  • MESON_NUM_PROCESSES respected every time Meson invokes multiple processes in parallel (supersedes MESON_TESTTHREADS
  • clang-tidy targets respect b_colorout
  • clang-tidy target do not mix output from multiple files
  • target introspection gains a machine entry for each sources group, allowing cross lookup from targets to compilers

mesonbuild/compilers/rust.py Fixed Show fixed Hide fixed
mesonbuild/scripts/clippy.py Fixed Show fixed Hide fixed
@bonzini bonzini force-pushed the clippy branch 2 times, most recently from 9b4a5e0 to cb0a22f Compare November 18, 2024 11:01
@bonzini bonzini marked this pull request as ready for review November 18, 2024 11:20
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

Generally looks good. Thanks for working on this.

mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
docs/markdown/IDE-integration.md Outdated Show resolved Hide resolved
mesonbuild/scripts/clippy.py Outdated Show resolved Hide resolved
mesonbuild/scripts/clippy.py Show resolved Hide resolved
mesonbuild/compilers/detect.py Outdated Show resolved Hide resolved
Copy link
Member

@dcbaker dcbaker left a comment

Choose a reason for hiding this comment

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

Thanks, this looks good to me.

@eli-schwartz
Copy link
Member

A couple of the CI failures could be cleared up with a rebase but the mypy lint workflow is also failing and that doesn't look like something external to this PR.

@bonzini
Copy link
Contributor Author

bonzini commented Nov 19, 2024

Hmm, it's a problem with TypedDict... Looks like I will have to make machine null.

@dcbaker
Copy link
Member

dcbaker commented Nov 19, 2024

@bonzini you can using typing_extensions.NotRequired[]

@bonzini
Copy link
Contributor Author

bonzini commented Nov 19, 2024

Thanks, and sorry for confusing the mypy failure with the msys ones.

@eli-schwartz
Copy link
Member

Restarting a potentially flaky job...

mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Show resolved Hide resolved
return 0

try:
await complete_all(futures)
Copy link
Member

Choose a reason for hiding this comment

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

... which leaves us with this as the one obviously useful import from mtest.py, which should potentially be refactored instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Or moved to utils, but I think a one-off is okay since these are the two places that use asyncio in Meson.

mesonbuild/compilers/rust.py Outdated Show resolved Hide resolved
mesonbuild/compilers/rust.py Outdated Show resolved Hide resolved
mesonbuild/scripts/run_tool.py Outdated Show resolved Hide resolved
docs/markdown/howtox.md Outdated Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
Buffering the output of clang-tidy will kill colored output, because the
tools being run are not going to see isatty() return true anymore.
To avoid that, pass b_colorout down to the clangtidy script.

As a bonus, this fixes -Db_colorout=never which was producing colored
output from clang-tidy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Differentiate from the "run_tool_on_targets" function that will be introduced
in the next commit.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
It is useful to apply a limit to the number of processes even outside "meson test",
and specifically for clang tools.  In preparation for this, generalize
determine_worker_count() to accept a variable MESON_NUM_PROCESSES instead of
MESON_TESTTHREADS, and use it throughout instead of multiprocessing.cpu_count().

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
This improves the handling of keyboard interrupt, and also makes it easy to
buffer the output and not mix errors from different subprocesses.  This
is useful for clang-tidy and will be used by clippy as well.  In addition,
the new code supports MESON_NUM_PROCESSES.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
There is no need to create and look up a dictionary when MachineChoice is
an enum, and there is no need to create a PerMachine object on every
__setitem__ of another PerMachine object.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Even though the "targets" introspection info already includes the
command line arguments used to invoke the compiler, this is not
enough to correlated with the "compilers" introspection info and
get extra information from there.

Together with the existing "language" key, adding a "machine" key
is enough to identify completely an entry in the compilers info.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Similar to the "ninja scan-build" target for C, add a clippy internal
tool that runs clippy-driver on all crates in the project.

The approach used is more efficient than with "ninja scan-build", and
does not require rerunning Meson in a separate build directory; it
uses the introspection data to find the compiler arguments for the
target and invokes clippy-driver with a slightly modified command
line.

This could actually be applied to scan-build as well, reusing the
run_tool_on_targets() function.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Add a target that builds all crates that could be extern to others,
and then reruns clippy.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
clippy-driver is not meant to be a general-purpose compiler front-end.
Since Meson can now provide natively the ability to invoke clippy,
raise a warning if someone uses it that way.

Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants