Rework rmake support library API #122460

jieyouxu · 2024-03-13T22:09:27Z

Take 1: Strongly-typed API

My 2 cents: from my experience with writing similar "test DSLs", I would suggest to create these helpers as soon as possible in the process (basically the first time someone needs them, not only after N similar usages), and basically treat any imperative code in these high-level tests as a maintenance burden, basically making them as declarative as possible. Otherwise it might be a bit annoying to keep refactoring the tests later once such helpers are available.

I would even discourage the arg method and create explicit methods for setting things like unpretty, the output file etc., but this might be more controversial, as it will make the invoked command-line arguments more opaque.

cc @Kobzol for the testing DSL suggestion.

Example:

let output = Rustc::new()
    .input_file("main.rs")
    .emit(&[EmitKind::Metadata])
    .extern_("stable", &stable_path)
    .output();

Take 2: xshell-based macro API

Example:

let sh = Shell::new()?;
let stable_path = stable_path.to_string_lossy();
let output = cmd!(sh, "rustc main.rs --emit=metadata --extern stable={stable_path}").output()?;

Take 3: Weakly-typed API with a few helper methods

let output = Rustc::new()
    .input("main.rs")
    .emit("metadata")
    .extern_("stable", &stable_path)
    .output();

rustbot · 2024-03-13T22:09:36Z

r? @Kobzol

rustbot has assigned @Kobzol.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

Noratrieb · 2024-03-14T07:37:36Z

tests/run-make/a-b-a-linker-guard/rmake.rs

+        .input_file("b.rs")
+        .enable_unstable_options()
+        .codegen_opt(PreferDynamic)
+        .codegen_opt(SymbolManglingVersion(SymbolManglingVersion::Legacy))


I'm not sure whether this really is better than what we had before (with merging the two like -Cprefer-dynamic into one line, which could be asserted by arg panicking when it sees -Z or -C).
downsides: it's more verbose and requires hardcoding the command line interface in the support library
upsides: 🤷 you get compile time instead of runtime checks?

Don't really have a super strong opinion for the support library API atm, still experimenting shrug

It does hardcode the cli interface into the support library. But if the cli interface changes, then surely the tests affected will have to change with it anyway?

Also I can omit the codegen_opt layer and just provide e.g. prefer_dynamic() / lto(LtoKind::Fat) directly.

I think arg is still simpler, easier to maintain and more obvious to understand

Noratrieb · 2024-03-14T07:40:21Z

tests/run-make/a-b-a-linker-guard/rmake.rs

-        .arg("symbol-mangling-version=legacy")
+    Rustc::new()
+        .input_file("a.rs")
+        .cfg("x")


I do like this helper, it makes the test shorter

klensy · 2024-03-14T08:49:44Z

Before: to write test you needed only to write almost as if bash command lines.
After: now you need to learn that DSL.

Maybe something like https://docs.rs/xshell/latest/xshell/ ? Used in r-a and looks cute.

Kobzol · 2024-03-14T09:24:20Z

Oh, I see you really went for it full steam ahead 😆 This approach is of course highly bike-sheddable and I'm sure that it will have both fans and people who don't like it.

Now that I look at it, I have to agree with Nilstrieb here that perhaps the strongly typed wrappers are too much, and they make it unnecessarily opaque to the reader - who probably knows the CLI flag, but might not know the wrapper. It's true that with the wrapper we can easily search for all usages of that flag in the run-make tests, but if you want to grep e.g. for all usages of -Zunpretty=hir (across all tests), then this actually makes that harder. So perhaps using the raw flags is a better choice after all.

On the other hand, I would keep some of the high-level helpers for things that are super common, but not very interesting and are "almost always boilerpate", for example the output -o parameter configuration (we could even auto-derive a default value for -o based on the input Rust filename, but that again makes the test more opaque).

Noratrieb · 2024-03-14T09:34:11Z

i agree that common things like -o deserve helpers, especially (or maybe rather only) if they make the code shorter/simpler.
another thing I'd suggest is making arg panic when it sees plain -C or -Z to force people to use -Za form instead of -Z a (as it makes the code significantly shorter and simpler).

jieyouxu · 2024-03-14T09:47:08Z

Before: to write test you needed only to write almost as if bash command lines. After: now you need to learn that DSL.

Maybe something like https://docs.rs/xshell/latest/xshell/ ? Used in r-a and looks cute.

I'm not sure why I thought it was a problem when I did the run-make infra PR initially, when I could've just re-exported xshell.

Now that I did write the super strongly-typed API, I think the shell version is just easier to read lol

extern run_make_support;
use run_make_support::anyhow;
use run_make_support::xshell::{Shell, cmd};

fn main() -> anyhow::Result<()> {
  let sh = Shell::new()?;
  let _blah = cmd!(sh, "rustc a.rs -Zunpretty=hir").read()?;
}

Maybe just go with the shell version after all? I want to get the API bikeshedding done before other tests are ported over :3

Kobzol · 2024-03-14T10:02:25Z

That approach could make it simpler to port the existing tests.

I would still keep some "context object", on which we can either call arg etc. for more complicated stuff, or just run the shell for simple use-cases. I'd definitely hide the usage of Shell behind our own API, to avoid exposing it directly to the test - if that is possible to do w.r.t. the macro.

In test DSLs, I often appreciate how I can do asserts on the return value of something, e.g.

let res = Command::new().set_arg(foo).run().assert_ok();
assert_eq!(res.get_output("bar"), "baz");

But I'm not actually sure if we need that in runmake tests, since we usually just invoke the compiler and then check the filesystem.

One thing that could be annoying with the shell approach is setting default values. E.g., we might want to set -o by default, or the rustc path by default, or redirect stdout/stderr of the compiler to some file, upon which we later want to do asserts etc. We should keep this option if we decide to use the shell. Replacing a Makefile with a Rust file that runs a fully unstructured and opaque shell command isn't such an improvement, I'd say :) We want to get rid of the shell-ism where possible.

jieyouxu · 2024-03-14T10:45:59Z

So the xshell version for a-b-a-linker-guard looks something like:

fn main() -> anyhow::Result<()> {
    let sh = Shell::new()?;
    let rustc = rustc();
    let out_dir = out_dir();
    cmd!(
        sh,
        "{rustc} a.rs --out-dir={out_dir} -L {out_dir} --cfg x -C prefer-dynamic \
        -Z unstable-options -C symbol-mangling-version=legacy"
    )
    .run()?;
    cmd!(
        sh,
        "{rustc} b.rs --out-dir={out_dir} -L {out_dir} -C prefer-dynamic -Z unstable-options \
        -C symbol-mangling-version=legacy"
    )
    .run()?;

    run("b");

    cmd!(
        sh,
        "{rustc} a.rs --out-dir={out_dir} -L {out_dir} --cfg y -C prefer-dynamic \
        -Z unstable-options -C symbol-mangling-version=legacy"
    )
    .run()?;

    run_fail("b");
}

jieyouxu · 2024-03-14T10:53:22Z

Now that I see how this version looks like... I think I personally prefer the typed API version, but with only a selected few helper methods (and not full-blown typed API) like Nils suggested. That way, we can still have presets (like aux_build et al.) and don't have to deal with a dependency on xshell.

jieyouxu · 2024-03-14T11:12:46Z

With the typed-but-not-too-strongly API:

fn main() {
    Rustc::new()
        .input("a.rs")
        .cfg("x")
        .arg("-Zunstable-options")
        .arg("-Cprefer-dynamic")
        .arg("-Csymbol-mangling-version=legacy")
        .run();

    Rustc::new()
        .input("b.rs")
        .arg("-Zunstable-options")
        .arg("-Cprefer-dynamic")
        .arg("-Csymbol-mangling-version=legacy")
        .run();

    run("b");

    Rustc::new()
        .input("a.rs")
        .cfg("y")
        .arg("-Zunstable-options")
        .arg("-Cprefer-dynamic")
        .arg("-Csymbol-mangling-version=legacy")
        .run();

    run_fail("b");
}

Kobzol · 2024-03-14T11:18:12Z

That looks like a good compromise to me. cfg will be less greppable due to this though (which is true basically for any helper method that we introduce on the rustc wrapper), but I don't think that a big issue for this specific flag.

workingjubilee · 2024-03-21T18:40:43Z

@rustbot ready

wait...

Noratrieb · 2024-03-21T18:46:19Z

tests/run-make/wasm-custom-section/rmake.rs

 use std::collections::HashMap;

 fn main() {
    if std::env::var("TARGET").unwrap() != "wasm32-wasip1" {
        return;
    }

-    rustc().arg("foo.rs").arg("--target=wasm32-wasip1").run();
-    rustc().arg("bar.rs").arg("--target=wasm32-wasip1").arg("-Clto").arg("-O").run();
+    Rustc::new().input("foo.rs").target("wasm32-wasip1").run();


Why rustc()->Rustc::new()?

Previously rustc() just called RustcInvocationBuilder::new() (which is now Rustc), it felt like an really unnecessary wrapper

I think it looks nicer

Noratrieb · 2024-03-21T18:46:50Z

I'm going to approve this, we can always change it later.
@bors r+

bors · 2024-03-21T18:46:52Z

📌 Commit 6ad3b4b has been approved by Nilstrieb

It is now in the queue for this repository.

…kingjubilee Rollup of 13 pull requests Successful merges: - rust-lang#121281 (regression test for rust-lang#103626) - rust-lang#121940 (Mention Register Size in `#[warn(asm_sub_register)]`) - rust-lang#122217 (Handle str literals written with `'` lexed as lifetime) - rust-lang#122379 (transmute: caution against int2ptr transmutation) - rust-lang#122460 (Rework rmake support library API) - rust-lang#122797 (Fix compile of wasm64-unknown-unknown target) - rust-lang#122875 (CFI: Support self_cell-like recursion) - rust-lang#122879 (CFI: Strip auto traits off Virtual calls) - rust-lang#122895 (add some ice tests 5xxxx to 9xxxx) - rust-lang#122907 (Uniquify `ReError` on input mode in canonicalizer) - rust-lang#122923 (In `pretty_print_type()`, print `async fn` futures' paths instead of spans.) - rust-lang#122942 (Add test in higher ranked subtype) - rust-lang#122963 (core/panicking: fix outdated comment) r? `@ghost` `@rustbot` modify labels: rollup

…=Nilstrieb Rework rmake support library API ### Take 1: Strongly-typed API Context: rust-lang#122448 (comment) > My 2 cents: from my experience with writing similar "test DSLs", I would suggest to create these helpers as soon as possible in the process (basically the first time someone needs them, not only after N similar usages), and basically treat any imperative code in these high-level tests as a maintenance burden, basically making them as declarative as possible. Otherwise it might be a bit annoying to keep refactoring the tests later once such helpers are available. > > I would even discourage the arg method and create explicit methods for setting things like unpretty, the output file etc., but this might be more controversial, as it will make the invoked command-line arguments more opaque. cc ``@Kobzol`` for the testing DSL suggestion. Example: ```rs let output = Rustc::new() .input_file("main.rs") .emit(&[EmitKind::Metadata]) .extern_("stable", &stable_path) .output(); ``` ### Take 2: xshell-based macro API Example: ```rs let sh = Shell::new()?; let stable_path = stable_path.to_string_lossy(); let output = cmd!(sh, "rustc main.rs --emit=metadata --extern stable={stable_path}").output()?; ``` ### Take 3: Weakly-typed API with a few helper methods ```rs let output = Rustc::new() .input("main.rs") .emit("metadata") .extern_("stable", &stable_path) .output(); ```

bors · 2024-03-24T04:43:28Z

🔒 Merge conflict

This pull request and the master branch diverged in a way that cannot be automatically merged. Please rebase on top of the latest master branch, and let the reviewer approve again.

How do I rebase?

Assuming self is your fork and upstream is this repository, you can resolve the conflict following these steps:

git checkout rmake-example-refactor (switch to your branch)
git fetch upstream master (retrieve the latest master)
git rebase upstream/master -p (rebase on top of it)
Follow the on-screen instruction to resolve conflicts (check git status if you got lost).
git push self rmake-example-refactor --force-with-lease (update this PR)

You may also read Git Rebasing to Resolve Conflicts by Drew Blessing for a short tutorial.

Please avoid the "Resolve conflicts" button on GitHub. It uses git merge instead of git rebase which makes the PR commit history more difficult to read.

Sometimes step 4 will complete without asking for resolution. This is usually due to difference between how Cargo.lock conflict is handled during merge and rebase. This is normal, and you should still perform step 5 to update this PR.

Error message

Auto-merging src/tools/run-make-support/src/lib.rs
CONFLICT (content): Merge conflict in src/tools/run-make-support/src/lib.rs
Automatic merge failed; fix conflicts and then commit the result.

bors · 2024-03-24T05:13:25Z

☔ The latest upstream changes (presumably #122966) made this pull request unmergeable. Please resolve the merge conflicts.

…ethods

jieyouxu · 2024-03-24T15:46:21Z

Changes since last reviewed:

Fixed merge conflicts.
Pull out rustc and rustdoc to their own modules.
Rename out_dir -> tmp_dir (this is the name tools.mk used).
Unshare setup logic because rustdoc should not have --out-dir set by default (run-make rustdoc tests seem to usually need to specify --out-dir themselves, and specifying --out-dir already would prevent overriding).
Introduce out_dir, input and arg_file helper methods on Rustdoc.

@rustbot ready

Noratrieb · 2024-03-27T16:09:31Z

@bors r+

bors · 2024-03-27T16:09:34Z

📌 Commit 1f2178b has been approved by Nilstrieb

It is now in the queue for this repository.

bors · 2024-03-27T17:43:24Z

⌛ Testing commit 1f2178b with merge 0157da4...

bors · 2024-03-27T19:57:37Z

☀️ Test successful - checks-actions
Approved by: Nilstrieb
Pushing 0157da4 to master...

rust-timer · 2024-03-28T01:26:06Z

Finished benchmarking commit (0157da4): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	-	-	0
Regressions ❌ (secondary)	3.6%	[1.3%, 6.0%]	2
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-	-	0

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.6%	[0.4%, 2.2%]	11
Regressions ❌ (secondary)	3.8%	[1.9%, 6.6%]	15
Improvements ✅ (primary)	-	-	0
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	1.6%	[0.4%, 2.2%]	11

Cycles

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

	mean	range	count
Regressions ❌ (primary)	1.9%	[1.9%, 1.9%]	1
Regressions ❌ (secondary)	4.2%	[4.2%, 4.2%]	1
Improvements ✅ (primary)	-0.7%	[-0.9%, -0.5%]	10
Improvements ✅ (secondary)	-	-	0
All ❌✅ (primary)	-0.4%	[-0.9%, 1.9%]	11

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: 670.379s -> 671.34s (0.14%)
Artifact size: 315.75 MiB -> 315.76 MiB (0.00%)

Kobzol · 2024-03-28T06:45:38Z

This only modified tests, so the result is noise.

@rustbot label: +perf-regression-triaged

rustbot assigned Kobzol Mar 13, 2024

rustbot added A-testsuite Area: The testsuite used to check the correctness of rustc S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 13, 2024

jieyouxu marked this pull request as draft March 13, 2024 22:09

jieyouxu force-pushed the rmake-example-refactor branch from a6f4f6a to 4acb17e Compare March 13, 2024 22:51

jieyouxu marked this pull request as ready for review March 13, 2024 22:51

jieyouxu added T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. and removed T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. labels Mar 13, 2024

Noratrieb reviewed Mar 14, 2024

View reviewed changes

jieyouxu changed the title ~~Rework rmake support library to use a testing DSL~~ [EXPERIMENTAL] Dogfood rmake support library API Mar 14, 2024

jieyouxu changed the title ~~[EXPERIMENTAL] Dogfood rmake support library API~~ [EXPERIMENTAL] Rework rmake support library API Mar 14, 2024

jieyouxu changed the title ~~[EXPERIMENTAL] Rework rmake support library API~~ Rework rmake support library API Mar 14, 2024

Noratrieb reviewed Mar 21, 2024

View reviewed changes

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 21, 2024

workingjubilee mentioned this pull request Mar 24, 2024

Rollup of 14 pull requests #122973

Closed

bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 24, 2024

Rework rmake support library to have a weakly-typed API with helper m…

1f2178b

…ethods

jieyouxu force-pushed the rmake-example-refactor branch from 4e8753b to 1f2178b Compare March 24, 2024 15:38

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 24, 2024

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 27, 2024

bors added the merged-by-bors This PR was explicitly merged by bors. label Mar 27, 2024

bors merged commit 0157da4 into rust-lang:master Mar 27, 2024
12 checks passed

rustbot added this to the 1.79.0 milestone Mar 27, 2024

This was referenced Mar 27, 2024

Fix error message for env! when env var is not valid Unicode #122663

Merged

Fix bug where option_env! would return None when env var is present but not valid Unicode #122670

Merged

rustbot added the perf-regression Performance regression. label Mar 28, 2024

rustbot added the perf-regression-triaged The performance regression has been triaged. label Mar 28, 2024

jieyouxu mentioned this pull request Mar 30, 2024

Rewrite core-no-fp-fmt-parse test in Rust #123180

Merged

jieyouxu deleted the rmake-example-refactor branch March 30, 2024 22:55

jieyouxu mentioned this pull request Mar 31, 2024

Show files produced by --emit foo in json artifact notifications #122597

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Rework rmake support library API #122460

Rework rmake support library API #122460

jieyouxu commented Mar 13, 2024 •

edited

Loading

rustbot commented Mar 13, 2024

Noratrieb Mar 14, 2024 •

edited

Loading

jieyouxu Mar 14, 2024

jieyouxu Mar 14, 2024

Noratrieb Mar 14, 2024

Noratrieb Mar 14, 2024

klensy commented Mar 14, 2024

Kobzol commented Mar 14, 2024 •

edited

Loading

Noratrieb commented Mar 14, 2024

jieyouxu commented Mar 14, 2024

Kobzol commented Mar 14, 2024

jieyouxu commented Mar 14, 2024

jieyouxu commented Mar 14, 2024 •

edited

Loading

jieyouxu commented Mar 14, 2024 •

edited

Loading

Kobzol commented Mar 14, 2024

workingjubilee commented Mar 21, 2024

Noratrieb Mar 21, 2024

jieyouxu Mar 21, 2024

Noratrieb Mar 21, 2024

Noratrieb commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 24, 2024

bors commented Mar 24, 2024

jieyouxu commented Mar 24, 2024 •

edited

Loading

Noratrieb commented Mar 27, 2024

bors commented Mar 27, 2024

bors commented Mar 27, 2024

bors commented Mar 27, 2024

rust-timer commented Mar 28, 2024

Kobzol commented Mar 28, 2024

Rework rmake support library API #122460

Rework rmake support library API #122460

Conversation

jieyouxu commented Mar 13, 2024 • edited Loading

Take 1: Strongly-typed API

Take 2: xshell-based macro API

Take 3: Weakly-typed API with a few helper methods

rustbot commented Mar 13, 2024

Noratrieb Mar 14, 2024 • edited Loading

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

Choose a reason for hiding this comment

klensy commented Mar 14, 2024

Kobzol commented Mar 14, 2024 • edited Loading

Noratrieb commented Mar 14, 2024

jieyouxu commented Mar 14, 2024

Kobzol commented Mar 14, 2024

jieyouxu commented Mar 14, 2024

jieyouxu commented Mar 14, 2024 • edited Loading

jieyouxu commented Mar 14, 2024 • edited Loading

Kobzol commented Mar 14, 2024

workingjubilee commented Mar 21, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Noratrieb commented Mar 21, 2024

bors commented Mar 21, 2024

bors commented Mar 24, 2024

bors commented Mar 24, 2024

jieyouxu commented Mar 24, 2024 • edited Loading

Noratrieb commented Mar 27, 2024

bors commented Mar 27, 2024

bors commented Mar 27, 2024

bors commented Mar 27, 2024

rust-timer commented Mar 28, 2024

Overall result: ❌ regressions - ACTION NEEDED

Kobzol commented Mar 28, 2024

jieyouxu commented Mar 13, 2024 •

edited

Loading

Noratrieb Mar 14, 2024 •

edited

Loading

Kobzol commented Mar 14, 2024 •

edited

Loading

jieyouxu commented Mar 14, 2024 •

edited

Loading

jieyouxu commented Mar 14, 2024 •

edited

Loading

jieyouxu commented Mar 24, 2024 •

edited

Loading