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

Allow running doctests via rust.doctest() #13933

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

Conversation

bonzini
Copy link
Collaborator

@bonzini bonzini commented Nov 20, 2024

Most of the series is refactoring that avoids duplicating the code that creates arguments to rustc. The integration with build.BuildTarget is a bit rust-specific, so I am willing to change things if you guys don't like it.

mesonbuild/build.py Fixed Show fixed Hide fixed
@@ -42,7 +42,7 @@
from .backend.backends import Backend
from .compilers import Compiler
from .interpreter.interpreter import SourceOutputs, Interpreter
from .interpreter.interpreterobjects import Test
from .interpreter.interpreterobjects import Test, Doctest

Check failure

Code scanning / CodeQL

Module-level cyclic import

'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](4) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](5) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](6) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](7) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](8) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](9) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](10) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](11) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](12) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](13) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](14) of mesonbuild.build. 'Test' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Test occurs after the cyclic [import](15) of mesonbuild.build.
@@ -42,7 +42,7 @@
from .backend.backends import Backend
from .compilers import Compiler
from .interpreter.interpreter import SourceOutputs, Interpreter
from .interpreter.interpreterobjects import Test
from .interpreter.interpreterobjects import Test, Doctest

Check failure

Code scanning / CodeQL

Module-level cyclic import

'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](4) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](5) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](6) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](7) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](8) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](9) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](10) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](11) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](12) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](13) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](14) of mesonbuild.build. 'Doctest' may not be defined if module [mesonbuild.interpreter.interpreterobjects](1) is imported before module [mesonbuild.build](2), as the [definition](3) of Doctest occurs after the cyclic [import](15) of mesonbuild.build.
@bonzini bonzini force-pushed the rustdoc branch 4 times, most recently from 06d0967 to 625e503 Compare November 20, 2024 16:28
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.

I didn't read things too closely, but here's some initial review.

mesonbuild/interpreter/interpreter.py Outdated Show resolved Hide resolved
mesonbuild/interpreter/interpreterobjects.py Outdated Show resolved Hide resolved
docs/markdown/Rust-module.md Outdated Show resolved Hide resolved
mesonbuild/compilers/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
@bonzini bonzini force-pushed the rustdoc branch 8 times, most recently from 3817dbd to 7b380be Compare December 12, 2024 17:30
@bonzini bonzini marked this pull request as ready for review December 19, 2024 17:53
@dcbaker
Copy link
Member

dcbaker commented Dec 19, 2024

As much to help me keep track of things: patches 1-5 (remove cratetype variable) are r-b

mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
mesonbuild/compilers/rust.py Outdated Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.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.

I've reviewed up to (including) ninjabackend: remove cratetype variable.

I've got a couple of small nits on those, but otherwise I'm happy for those to land. I'll try to get around to the rest later today

docs/markdown/Rust-module.md Outdated Show resolved Hide resolved
docs/markdown/Rust-module.md Show resolved Hide resolved
mesonbuild/compilers/rust.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.

Alright, I made it to the end before the EOD!

There's some minor stuff in here, but overall I think this is very close to being ready to land.

mesonbuild/modules/rust.py Show resolved Hide resolved
mesonbuild/backend/ninjabackend.py Outdated Show resolved Hide resolved
mesonbuild/compilers/rust.py Show resolved Hide resolved
mesonbuild/interpreter/kwargs.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
mesonbuild/modules/rust.py Outdated Show resolved Hide resolved
@bonzini bonzini force-pushed the rustdoc branch 3 times, most recently from cef6179 to defbf2b Compare February 3, 2025 09:04
@eli-schwartz
Copy link
Member

I've reviewed up to (including) ninjabackend: remove cratetype variable.

Same. Let's push these preliminary fixups now: b825fae...d34c37f

Allow reusing the code for doctests.  In particular, the sources are
shared between the two cases.

Signed-off-by: Paolo Bonzini <[email protected]>
This will be used by rustdoc tests because the Test objects takes a
single string for the command and everything else goes in the args.
But apart from this, the need to split the executable from the
arguments is common so create new methods to do it.

While at it, fix brokenness in the handling of the zig compiler, which
is checking against "zig" but failing to detect e.g. "/usr/bin/zig".

Signed-off-by: Paolo Bonzini <[email protected]>
A doctest target is a separate build target (with its own linker
arguments, including dependencies) that is built and added as a
unit test whenever the parent target is built.  The doctest's
target is not accessible via ninja.

Signed-off-by: Paolo Bonzini <[email protected]>
Adjust get_rust_compiler_args() to accept the crate-type externally, because
rustdoc tests are an executable but are compiled with the parent target's
--crate-type.

Apart from that, the rustdoc arguments are very similar to the parent target, and
are handled by the same functions that were split out of generate_rust_target.

This concludes the backend implementation of doctests, only leaving the
implementation of a doctest() function in the rust module.

Signed-off-by: Paolo Bonzini <[email protected]>
rust.doctest() will have to typecheck that to a list of strings,
no other argument types are allowed.  Extract the field out of
BaseTest, placing it in FuncBenchmark and the rust modules's
FuncTest.

Signed-off-by: Paolo Bonzini <[email protected]>
Signed-off-by: Paolo Bonzini <[email protected]>
QEMU needs it in its integration tests (in order to run global constructors),
and therefore in rust.doctest too.  With this change I could do:

  # Rust executables do not support objects, so add an intermediate step.
  rust_qemu_api_objs = static_library(
    'rust_qemu_api_objs',
    objects: [libqom.extract_all_objects(recursive: false),
              libhwcore.extract_all_objects(recursive: false)])

  rust.doctest('rust-qemu-api-doc', _qemu_api_rs,
    dependencies: [qemu_api, qemu_api_macros],
    link_with: libqemuutil,
    link_whole: [rust_qemu_api_objs],
    suite: ['doc', 'rust'])

followed by "meson test --suite doc".

For completeness, add it to rust.test as well.

Signed-off-by: Paolo Bonzini <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
module:rust Specific to the Rust module
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants