Fetch miri flags from cargo config #2451

avrong · 2022-07-29T21:29:17Z

This PR introduces cargo config parsing, and in a current implementation uses miri.flags property if there is no MIRIFLAGS env variable present. Maybe the best way to do that is to have some kind of merge of the parameters in both configs, sort of like cargo configs do it by themselves depending on location.

I did not make other tests, nor changed the docs, would be happy to do that if it's better to have them :)
Didn't figure out yet how to make a independent test to check the handling of .cargo/config.toml flags.

RalfJung · 2022-07-30T03:41:55Z

Thanks for the PR!

What does cargo do when RUSTFLAGS is set and there are also flags set via config.toml? We should do the same thing.

And yes this should also be documented. :) Tests could be a bit tricky, do you have ideas for that?

avrong · 2022-07-31T13:10:27Z

Regarding RUSTFLAGS, it does the same thing. Exclusively chooses one of the sources in the order.

Thinking of tests, as I understand we can simply test this by making a cargo project where cargo miri would be failing if one of the flags is not present, which we will define in .cargo/config.toml. I see that we can use e.g. some I/O operation such as opening a file and specifying -Zmiri-disable-isolation.

cargo-miri/bin.rs

RalfJung · 2022-08-02T19:29:36Z

Thinking of tests, as I understand we can simply test this by making a cargo project where cargo miri would be failing if one of the flags is not present, which we will define in .cargo/config.toml. I see that we can use e.g. some I/O operation such as opening a file and specifying -Zmiri-disable-isolation.

Our tests for cargo-miri are in test-cargo-miri/run-test.py. cargo miri run in that project needs isolation disabled so I guess you could

remove .cargo/config.toml at the beginning of test_cargo_miri_run (just in case of stray files)
after the first test, create a .cargo/config.toml that sets isolation`
run the test again without MIRIFLAGS, which would then test that the config.toml is honored.
clean up .cargo/config.toml again

RalfJung

Thanks for the PR. :) I just have some minor nits.

RalfJung · 2022-08-03T15:04:14Z

@rustbot author
(use @rustbot review when this is ready for review again)

bors · 2022-08-03T15:43:18Z

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

avrong · 2022-08-04T03:53:18Z

@rustbot review

I also added alongside others one test for checking the priority between MIRIFLAGS envar and miri.flags setting in the cargo config.

cargo-miri/src/phases.rs

test-cargo-miri/run-test.py

RalfJung · 2022-08-06T21:41:01Z

@rustbot author

RalfJung · 2022-08-06T23:24:37Z

cargo-miri/src/phases.rs

@@ -541,6 +541,27 @@ pub fn phase_runner(mut binary_args: impl Iterator<Item = String>, phase: Runner
    }
 }

+fn get_miriflags() -> Option<Vec<String>> {
+    // Fetch miri flags from cargo config.
+    let mut cmd = cargo();


I think we need to do a bit more work here: the user might have passed --config ... to cargo miri run, which would be ignored this way. See get_cargo_metadata for how to forward that. Also they might have set the manifest path, which even get_cargo_metadata gets wrong.

Once #2474 lands there will be a function cargo_extra_flags you can call to get all the flags you need.

However, you can only call that function in phase_cargo_miri; the arguments are lost here in phase_runner. So you'll need to do the flag handling there, and then put the results into an env var that is picked up again in phase_runner.

I think the "right" way to do that is to have a variable like CARGO_ENCODED_MIRIFLAGS. Then phase_cargo_miri can determine the flags (from either MIRIFLAGS or via the config -- or from CARGO_ENCODED_MIRIFLAGS if that is already set -- and then store the flags losslessly (without spaces causing trouble) into CARGO_ENCODED_MIRIFLAGS here phase_runner can retreive it.

If that's more work than what you signed up for, then storing it into MIRIFLAGS and not supporting spaces within arguments for now would also work.

RalfJung · 2022-08-07T02:38:27Z

test-cargo-miri/run-test.py

@@ -90,6 +90,12 @@ def test_no_rebuild(name, cmd, env={}):
        fail("Something was being rebuilt when it should not be (or we got no output)");

 def test_cargo_miri_run():
+    # Try to remove cargo config to avoid unexpected settings
+    try:
+        os.remove('.cargo/config.toml')


I just realized this seems to edit the same file as

miri/ci.sh

Line 49 in aa53f3f

echo 'build.rustc-wrapper = "thisdoesnotexist"' > .cargo/config.toml

...

Oh yeah, the path should be absolute here

That's not what I mean. It can be relative, the other paths in the python file are also relative.

What I mean is that removing that file removes testing the build.rustc-wrapper = "thisdoesnotexist" configuration!

RalfJung · 2022-09-22T08:45:09Z

@avrong we haven't heard from you in a while; what is the status of this PR?

avrong · 2022-09-22T12:42:28Z

@RalfJung sorry for a long delay. Will be looking at this again soon

RalfJung · 2022-10-26T09:00:25Z

@avrong I'm going to close this due to inactivity, but feel free to pick it up again any time!

badumbatish · 2024-09-08T07:32:25Z

i'll study the PR and initial issue on this

RalfJung reviewed Aug 2, 2022

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 2, 2022

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 2, 2022

View reviewed changes

cargo-miri/bin.rs Outdated Show resolved Hide resolved

RalfJung reviewed Aug 2, 2022

View reviewed changes

rustbot added the S-waiting-on-author Status: Waiting for the PR author to address review comments label Aug 3, 2022

avrong force-pushed the cargo-config branch from abf0948 to 96b9655 Compare August 4, 2022 03:26

avrong added 3 commits August 4, 2022 06:30

Fetch miri flags from cargo config

7e32c0f

Add tests for fetching flags from from cargo config

f0559b9

Add info on setting miriflags in cargo config

56ee267

avrong force-pushed the cargo-config branch from 96b9655 to 56ee267 Compare August 4, 2022 03:31

rustbot added S-waiting-on-review Status: Waiting for a review to complete and removed S-waiting-on-author Status: Waiting for the PR author to address review comments labels Aug 4, 2022

RalfJung reviewed Aug 6, 2022

View reviewed changes

cargo-miri/src/phases.rs Show resolved Hide resolved

RalfJung reviewed Aug 6, 2022

View reviewed changes

cargo-miri/src/phases.rs Show resolved Hide resolved

RalfJung reviewed Aug 6, 2022

View reviewed changes

test-cargo-miri/run-test.py Show resolved Hide resolved

rustbot added S-waiting-on-author Status: Waiting for the PR author to address review comments and removed S-waiting-on-review Status: Waiting for a review to complete labels Aug 6, 2022

RalfJung reviewed Aug 6, 2022

View reviewed changes

RalfJung reviewed Aug 7, 2022

View reviewed changes

RalfJung closed this Oct 26, 2022

RalfJung mentioned this pull request Oct 26, 2022

Set MIRIFLAGS from .cargo/config.toml #2347

Open

RossSmyth mentioned this pull request Feb 25, 2024

Ship Miri on stable toolchains #3322

Open

9 tasks

badumbatish mentioned this pull request Oct 8, 2024

Add a way to extract miri flags from --config, env and toml #3875

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fetch miri flags from cargo config #2451

Fetch miri flags from cargo config #2451

avrong commented Jul 29, 2022 •

edited

Loading

RalfJung commented Jul 30, 2022

avrong commented Jul 31, 2022

RalfJung commented Aug 2, 2022

RalfJung left a comment

RalfJung commented Aug 3, 2022

bors commented Aug 3, 2022

avrong commented Aug 4, 2022

RalfJung commented Aug 6, 2022

RalfJung Aug 6, 2022

RalfJung Aug 7, 2022

RalfJung Aug 7, 2022

RalfJung Aug 7, 2022

avrong Aug 10, 2022

RalfJung Aug 10, 2022 •

edited

Loading

RalfJung commented Sep 22, 2022

avrong commented Sep 22, 2022

RalfJung commented Oct 26, 2022

badumbatish commented Sep 8, 2024

Fetch miri flags from cargo config #2451

Fetch miri flags from cargo config #2451

Conversation

avrong commented Jul 29, 2022 • edited Loading

RalfJung commented Jul 30, 2022

avrong commented Jul 31, 2022

RalfJung commented Aug 2, 2022

RalfJung left a comment

Choose a reason for hiding this comment

RalfJung commented Aug 3, 2022

bors commented Aug 3, 2022

avrong commented Aug 4, 2022

RalfJung commented Aug 6, 2022

RalfJung Aug 6, 2022

Choose a reason for hiding this comment

RalfJung Aug 7, 2022

Choose a reason for hiding this comment

RalfJung Aug 7, 2022

Choose a reason for hiding this comment

RalfJung Aug 7, 2022

Choose a reason for hiding this comment

avrong Aug 10, 2022

Choose a reason for hiding this comment

RalfJung Aug 10, 2022 • edited Loading

Choose a reason for hiding this comment

RalfJung commented Sep 22, 2022

avrong commented Sep 22, 2022

RalfJung commented Oct 26, 2022

badumbatish commented Sep 8, 2024

avrong commented Jul 29, 2022 •

edited

Loading

RalfJung Aug 10, 2022 •

edited

Loading