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

Tracking Issue for porting run-make tests to use Rust #121876

Open
jieyouxu opened this issue Mar 1, 2024 · 37 comments
Open

Tracking Issue for porting run-make tests to use Rust #121876

jieyouxu opened this issue Mar 1, 2024 · 37 comments
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@jieyouxu
Copy link
Member

jieyouxu commented Mar 1, 2024

We want to stop run-make tests from relying on make, and improve the run-make tests so that
they are more accessible to rustc contributors by allowing the tests to be written in Rust (see
#40713 for context). PR #113026 was merged to address this, and now we are able to write run-make
tests in Rust recipes. We would like your help to port over existing run-make tests still using
Makefiles to use Rust recipes instead.

If you would like to work on porting one of the run-make tests, please link to this issue in your
PR and leave a comment to claim the test (or multiple tests). If you are stuck, please don't hesitate to open a thread
on Rust's Zulip.

run-make is the most flexible fallback test kind, but some run-make tests could be migrated to become e.g. ui tests instead. Please check if you can convert the run-make test into other test kinds before porting!

When you try to port a test, also consider:

  • Add some comments on what the test is trying to test.
  • Add some comments on how the test tries to accomplish its goals if the test is non-trivial. This is especially true if you discovered that a test has platform/compiler/architecture/tooling-specific behavior that is a pain to debug.
  • Any related issues? Any relevant links?
  • Is the test still applicable? Has it become outdated? Is it duplicated?
  • Can it be written in other test suites?

Context: Rust recipes?

PR #113026 adds basic infrastructure support to write run-make tests using small Rust programs,
called recipes.

We aim to eliminate the dependency on make and Makefiles for building run-make-style tests.
Makefiles are replaced by recipes (rmake.rs). The PR implements running run-make recipes in
3 steps:

  1. We build the support library run_make_support which the rmake.rs recipes depend on as a tool
    lib.
  2. We build the recipe rmake.rs and link in the support library.
  3. We run the recipe to build and run the tests.

rmake.rs is basically a replacement for Makefile, and allows running arbitrary Rust code. The
support library is built using cargo, and so can depend on external crates if desired.

The infrastructure implemented by the PR is very barebones, and is the minimally required
infrastructure needed to build, run and pass the two example run-make tests ported over to the new
infrastructure:

You likely will find that you would need to improve the API of the support library, and extend
the functionality of the support library.

Common traps and pitfalls, and tips and tricks

  • $PATH uses ; on Windows and : for *nixes. Use std::env::{join,split}_paths to properly
    handle $PATH.

  • Be careful of path separator platform differences. Always prefer PathBuf operations not string
    paths if possible.

  • tests/ are not (currently) formatted by rustfmt.

  • Consult /~https://github.com/rust-lang/rust/blob/master/tests/run-make/tools.mk for which flags
    and envs are passed to various executables or libraries. May have to triple check on those.

  • You can always request your reviewer to run try jobs to test out your PR on environments you don't locally have access to. Good candidate CI jobs include:

    • aarch64-apple (apple, 64 bits, aarch64/arm64)
    • armhf-gnu (cross-compile)
    • test-various (cross-compile, wasm)
    • x86_64-mingw (x86_64, 64 bits, windows, mingw)
    • x86_64-msvc (x86_64, 64 bits, windows, msvc)
    • x86_64-gnu-llvm-18 (x86_64, 64 bits, windows, gnu, llvm 18)
    • i686-msvc (x86, 32 bits, windows, msvc)

Tests that need porting

@jieyouxu jieyouxu added A-testsuite Area: The testsuite used to check the correctness of rustc T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. E-help-wanted Call for participation: Help is requested to fix this issue. C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC labels Mar 1, 2024
@5225225

This comment was marked as resolved.

@abhay-51

This comment was marked as resolved.

@high-cloud

This comment was marked as resolved.

@0xhiro

This comment was marked as resolved.

@JoverZhang

This comment was marked as resolved.

@workingjubilee workingjubilee added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Mar 22, 2024
@Oneirical

This comment was marked as resolved.

@Oneirical

This comment was marked as resolved.

workingjubilee added a commit to workingjubilee/rustc that referenced this issue Mar 31, 2024
Rewrite `core-no-fp-fmt-parse` test in Rust

Claiming the simple "core-no-fp-fmt-parse" test from rust-lang#121876. `run_make_support` was altered with `arg_path` written in rust-lang#121918 by `@abhay-51,` with additional doc comment.

Preliminary GSoC contribution for the project proposal mentored by `@jieyouxu.`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Mar 31, 2024
Rollup merge of rust-lang#123180 - Oneirical:master, r=Mark-Simulacrum

Rewrite `core-no-fp-fmt-parse` test in Rust

Claiming the simple "core-no-fp-fmt-parse" test from rust-lang#121876. `run_make_support` was altered with `arg_path` written in rust-lang#121918 by `@abhay-51,` with additional doc comment.

Preliminary GSoC contribution for the project proposal mentored by `@jieyouxu.`
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this issue Apr 4, 2024
Port hir-tree run-make test to ui test

As part of rust-lang#121876

cc `@jieyouxu`
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 4, 2024
Rollup merge of rust-lang#122448 - high-cloud:move-hir-tree, r=oli-obk

Port hir-tree run-make test to ui test

As part of rust-lang#121876

cc `@jieyouxu`
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Apr 5, 2024
…e-enum, r=Mark-Simulacrum

Port argument-non-c-like-enum to Rust

Part of rust-lang#121876.
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this issue Apr 5, 2024
…imulacrum

Port `run-make/issue-7349` to a codegen test

The test does not need to be a run-make test, it can use the codegen test infrastructure.

Also took the opportunity to rename the test to `no-redundant-item-monomorphization` so it's not just some opaque issue number.

Part of rust-lang#121876.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 5, 2024
Rollup merge of rust-lang#123474 - jieyouxu:issue-7349-port, r=Mark-Simulacrum

Port `run-make/issue-7349` to a codegen test

The test does not need to be a run-make test, it can use the codegen test infrastructure.

Also took the opportunity to rename the test to `no-redundant-item-monomorphization` so it's not just some opaque issue number.

Part of rust-lang#121876.
rust-timer added a commit to rust-lang-ci/rust that referenced this issue Apr 5, 2024
Rollup merge of rust-lang#123149 - jieyouxu:rmake-arguments-non-c-like-enum, r=Mark-Simulacrum

Port argument-non-c-like-enum to Rust

Part of rust-lang#121876.
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 13, 2025
…, r=<try>

tests: Port `extern-fn-reachable` to rmake.rs

Part of rust-lang#121876.

## Summary

This PR ports `tests/run-make/extern-fn-reachable` to use `rmake.rs`. Notable changes:

- We now use the `object` crate and look at the exported symbols specifically.
- This test's coverage regressed against windows-msvc back in [replace dynamic library module with libloading rust-lang#90716](rust-lang#90716), but since we use `object` now, we're able to claw the test coverage back.
- The checks are now stricter:
    1. It no longer looks for substring symbol matches in `nm` textual outputs, it inspects the symbol names precisely.
    2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.

## History

- Test was initially introduced as a run-pass[^run-pass] test as part of [Don't mark reachable extern fns as internal rust-lang#10539](rust-lang#10539).
- Test re-introduced as a run-make test in rust-lang#13741.
- Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314.
Co-authored with `@lolbinarycat.`

r? `@ghost`

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 13, 2025
…=<try>

tests: Port `jobserver-error.rs` to rmake.rs

Part of rust-lang#121876.

This PR ports `jobserver-error.rs` to rmake.rs, and is basically rust-lang#128789 slightly adjusted. Namely, `set_aux_fd` is made `unsafe`, alongside some doc updates.

The complexity involved here is mostly how to get `/dev/null/` piping to fd 3 working with std `Command`, whereas with a shell this is much easier (as is evident with the `Makefile` version).

Supersedes rust-lang#128789.
This PR is co-authored with `@Oneirical` and `@coolreader18.`

r? `@ghost`

try-job: aarch64-gnu
try-job: i686-gnu-1
try-job: x86_64-gnu-debug
try-job: x86_64-gnu-llvm-18-1
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 14, 2025
…=<try>

tests: Port `jobserver-error` to rmake.rs

Part of rust-lang#121876.

This PR ports `tests/run-make/jobserver-error` to rmake.rs, and is basically rust-lang#128789 slightly adjusted. Namely, `set_aux_fd` is made `unsafe`, alongside some doc updates.

The complexity involved here is mostly how to get `/dev/null/` piping to fd 3 working with std `Command`, whereas with a shell this is much easier (as is evident with the `Makefile` version).

Supersedes rust-lang#128789.
This PR is co-authored with `@Oneirical` and `@coolreader18.`

r? `@ghost`

try-job: aarch64-gnu
try-job: i686-gnu-1
try-job: x86_64-gnu-debug
try-job: x86_64-gnu-llvm-18-1
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 14, 2025
…, r=<try>

tests: Port `extern-fn-reachable` to rmake.rs

Part of rust-lang#121876.

## Summary

This PR ports `tests/run-make/extern-fn-reachable` to use `rmake.rs`. Notable changes:

- We now use the `object` crate and look at the exported symbols specifically.
- This test's coverage regressed against windows-msvc back in [replace dynamic library module with libloading rust-lang#90716](rust-lang#90716), but since we use `object` now, we're able to claw the test coverage back.
- The checks are now stricter:
    1. It no longer looks for substring symbol matches in `nm` textual outputs, it inspects the symbol names precisely.
    2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.
- Added another case of `#[no_mangle] fn fun6() {}` (note the lack of `pub`) to check that Rust nameres visibility is orthogonal to symbol visiblity in dylib.

## History

- Test was initially introduced as a run-pass[^run-pass] test as part of [Don't mark reachable extern fns as internal rust-lang#10539](rust-lang#10539).
- Test re-introduced as a run-make test in rust-lang#13741.
- Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314.
Co-authored with `@lolbinarycat.`

r? `@ghost`

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 16, 2025
…, r=lqd

tests: Port `extern-fn-reachable` to rmake.rs

Part of rust-lang#121876.

## Summary

This PR ports `tests/run-make/extern-fn-reachable` to use `rmake.rs`. Notable changes:

- We now use the `object` crate and look at the exported symbols specifically.
- This test's coverage regressed against windows-msvc back in [replace dynamic library module with libloading rust-lang#90716](rust-lang#90716), but since we use `object` now, we're able to claw the test coverage back.
- The checks are now stricter:
    1. It no longer looks for substring symbol matches in `nm` textual outputs, it inspects the symbol names precisely.
    2. We now also explicitly check for the presence of leading underscore in exported symbol names on apple vs non-apple targets.
- Added another case of `#[no_mangle] fn fun6() {}` (note the lack of `pub`) to check that Rust nameres visibility is orthogonal to symbol visibility in dylib.

## History

- Test was initially introduced as a run-pass[^run-pass] test as part of [Don't mark reachable extern fns as internal rust-lang#10539](rust-lang#10539).
- Test re-introduced as a run-make test in rust-lang#13741.
- Later, the test coverage regressed in rust-lang#90716.

[^run-pass]: no longer a thing nowadays

Supersedes rust-lang#128314.
Co-authored with `@lolbinarycat.`

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 16, 2025
…<try>

tests: Port `split-debuginfo` to rmake.rs

Part of rust-lang#121876.

## Changes

This PR ports `tests/run-make/split-debuginfo` to rmake.rs. This is an **initial** port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I made

1. The linux/non-linux final branch had a conditional interpolation of `UNSTABLE_OPTIONS := -Zunstable-options`. However, one of the use sites was `-C $(UNSTABLE_OPTIONS) split-debuginfo`. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to `-C -Zunstable-options split-debuginfo`. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.
2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.
3. The check for path remapping / lack of path remapping through

    ```make
    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1
    ```

    are incorrect, because that looks at the single line of that contains `DW_AT_GNU_dwo_name`. This is unfortunately wrong because empirical evidence shows that with `objdump`[^objdump], the check actually needs to look at the attribute value of `DW_AT_comp_dir` on the previous line not `DW_AT_GNU_dwo_name`[^gnu-ext]. Example output of `objdump`:

	```text
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
	```

	In the rmake.rs version
4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to do them in this initial port[^enhancement].
5. The Makefile version didn't test *anything* on Windows (both windows-msvc and windows-gnu). I added some *very* basic and *very* sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.
6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

[^objdump]: the output format differs between `objdump` and `llvm-objdump`, but the same is true for `llvm-objdump` that this is looking at the wrong line.
[^gnu-ext]: AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec
[^enhancement]: For instance, the previous path remapping check could in theory be precisely inspected by inspecting `.debug_info` section to look for attribute value of `DW_AT_comp_dir`. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use *that* offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace `llvm-objdump` textual output substring matches with more precise `object` + `gimli` inspections.

## Review advice

- I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting.
- This PR *intentionally* introduces several intermediate commits for the `Makefile`, mostly to illustrate the problems I discovered when looking at the original `Makefile` version. This is intended to highlight the existing problems in the `Makefile` version for the reviewer[^squash].
    - There are several intentional non-functional commits:
        1. Reindent the `Makefile` to make the platform conditional gating more obvious.
        2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
        3. Remove all redundant `-C debuginfo=2` when `-g` is already specified.
- This PR is best reviewed commit-by-commit.

[^squash]: I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

---

This PR supersedes rust-lang#128754 and is co-authored with `@Oneirical.`

r? `@ghost`

---

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
bors added a commit to rust-lang-ci/rust that referenced this issue Jan 16, 2025
…<try>

tests: Port `split-debuginfo` to rmake.rs

Part of rust-lang#121876.

## Changes

This PR ports `tests/run-make/split-debuginfo` to rmake.rs. This is an **initial** port, and certainly could be cleaned up and/or enhanced.

The original Makefile version had several functional problems. I made

1. The linux/non-linux final branch had a conditional interpolation of `UNSTABLE_OPTIONS := -Zunstable-options`. However, one of the use sites was `-C $(UNSTABLE_OPTIONS) split-debuginfo`. This indicates to me that this run-make test is not run in CI under a non-linux + non-windows + non-darwin environment, because that would've failed as this would expand to `-C -Zunstable-options split-debuginfo`. I fixed this in the rmake.rs version, but I'm not sure if this distinction is worth keeping at all if it's not tested in CI.
2. There are several comments that were discovered to be wrong. I tried to fix them in the rmake.rs version as well.
3. The check for path remapping / lack of path remapping through

    ```make
    objdump -Wi $(TMPDIR)/foo | grep DW_AT_GNU_dwo_name | (! grep $(TMPDIR)) || exit 1
    ```

    are incorrect, because that looks at the single line of that contains `DW_AT_GNU_dwo_name`. This is unfortunately wrong because empirical evidence shows that with `objdump`[^objdump], the check actually needs to look at the attribute value of `DW_AT_comp_dir` on the previous line not `DW_AT_GNU_dwo_name`[^gnu-ext]. Example output of `objdump`:

	```text
    <10>   DW_AT_comp_dir    : (indirect string, offset: 0xafb48): /home/joe/repos/rust
    <14>   DW_AT_GNU_dwo_name: (indirect string, offset: 0x5d1b0): foo.foo.fc848df41df7a00d-cgu.0.rcgu.dwo
	```

	In the rmake.rs version
4. I included a bunch of FIXMEs and ENHANCEMENTs I noticed regarding the test because I didn't want to do them in this initial port[^enhancement].
5. The Makefile version didn't test *anything* on Windows (both windows-msvc and windows-gnu). I added some *very* basic and *very* sparse checks for windows-msvc, but I am not willing to spend the effort to expand test coverage to windows-gnu in this initial port.
6. This run-make test is way too big. But I didn't want to expend the effort of breaking this up in this initial port.

[^objdump]: the output format differs between `objdump` and `llvm-objdump`, but the same is true for `llvm-objdump` that this is looking at the wrong line.
[^gnu-ext]: AFAICT that is a GNU DWARF attribute extension, since it isn't mentioned in DWARFv5 spec
[^enhancement]: For instance, the previous path remapping check could in theory be precisely inspected by inspecting `.debug_info` section to look for attribute value of `DW_AT_comp_dir`. But that involves resolving the value of the indirect string, which means you have to: (1) look for offset into string offset table and (2) use *that* offset to find the string itself in the string table. The split part of "split-debuginfo" makes this murky for me, so I wasn't able to replace `llvm-objdump` textual output substring matches with more precise `object` + `gimli` inspections.

## Review advice

- I'm sorry for how long the rmake.rs test ended up, but a lot of it is comments and just vertical space due to formatting.
- This PR *intentionally* introduces several intermediate commits for the `Makefile`, mostly to illustrate the problems I discovered when looking at the original `Makefile` version. This is intended to highlight the existing problems in the `Makefile` version for the reviewer[^squash].
    - There are several intentional non-functional commits:
        1. Reindent the `Makefile` to make the platform conditional gating more obvious.
        2. Collapse nested if-else branches into an else if construct, which is not supported by GNU Make 3.80.
        3. Remove all redundant `-C debuginfo=2` when `-g` is already specified.
- This PR is best reviewed commit-by-commit.

[^squash]: I intend to squash these intermediate commits away after the reviewer concludes that the current form of the rmake.rs test is acceptable for merge. Before then, I'll keep them to help with review.

---

This PR supersedes rust-lang#128754 and is co-authored with `@Oneirical.`

r? `@ghost`

---

try-job: x86_64-msvc
try-job: i686-msvc
try-job: i686-mingw
try-job: x86_64-mingw-1
try-job: x86_64-apple-1
try-job: aarch64-apple
try-job: test-various
@jieyouxu
Copy link
Member Author

jieyouxu commented Jan 16, 2025

Re-triage (2025-01-16):

Test Latest PR Status
cat-and-grep-sanity-check #134762 Helper test. Should only delete when there are no longer any Makefiles.
jobserver-error #135461 jobserver stuff... tricky, waiting on review. New PR #135461
split-debuginfo #135572 New PR #135572
symbol-mangling-hashed #128567 Waiting for me to get to it
translation #129011 Waiting for me to get to it

Also need to:

  • Remove tools.mk after all Makefiles are gone.
  • Remove support for legacy Makefile run-make infra
  • Update run-make docs to remove any mention of the Makefile infra

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-run-make Area: port run-make Makefiles to rmake.rs A-testsuite Area: The testsuite used to check the correctness of rustc C-tracking-issue Category: An issue tracking the progress of sth. like the implementation of an RFC T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
Status: In progress
Development

No branches or pull requests

15 participants