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.

Sign up for GitHub

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

Jump to bottom

[cargo-miri] Skip unit tests of proc-macro crates #1675

Merged
merged 3 commits into from Jan 24, 2021
Merged

[cargo-miri] Skip unit tests of proc-macro crates #1675

merged 3 commits into from Jan 24, 2021

Conversation

ghost
Copy link

@ghost ghost commented Jan 15, 2021

Fixes #1660.

@RalfJung
Copy link
Member

Thanks for the PR! I am surprised that this works even for foreign-arch builds.

However, this seems like an ad-hoc work-around. I'd really prefer it we could make cargo-miri properly detect this situation upfront, instead of having to "change its mind" when the JSON read failed.

@RalfJung RalfJung mentioned this pull request Jan 15, 2021
Closed
@ghost

This comment has been minimized.

Sign in to view
@ghost ghost mentioned this pull request Jan 16, 2021
Closed
ghost
ghost commented Jan 16, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

I thought about this some more, and I don't think we should go with this approach. The expectation with cargo miri test is that tests are run in Miri. This PR violates that expectation. I think it would be better to not execute the tests at all than to silently run them outside of Miri.

Does that make sense?

@ghost
Copy link
Author

ghost commented Jan 18, 2021

I thought about this some more, and I don't think we should go with this approach. The expectation with cargo miri test is that tests are run in Miri. This PR violates that expectation. I think it would be better to not execute the tests at all than to silently run them outside of Miri.

Does that make sense?

I think that's OK. I think people often run both cargo test and cargo miri test, so skipping these tests seems fine.

@ghost ghost closed this Jan 18, 2021
@ghost ghost deleted the proc-macro-unit-test branch January 18, 2021 09:55
@ghost ghost restored the proc-macro-unit-test branch January 18, 2021 09:55
@ghost

This comment has been minimized.

Sign in to view
@ghost ghost reopened this Jan 18, 2021
@ghost ghost changed the title [cargo-miri] Execute the binary when it's not JSON [cargo-miri] Skip unit tests of proc-macro crates Jan 18, 2021
@ghost
Copy link
Author

ghost commented Jan 18, 2021 •
edited by ghost
Loading

I thought about this some more, and I don't think we should go with this approach. The expectation with cargo miri test is that tests are run in Miri. This PR violates that expectation. I think it would be better to not execute the tests at all than to silently run them outside of Miri.

Does that make sense?

This now skips these tests instead of changing its mind when running them.

ghost
ghost commented Jan 18, 2021
View reviewed changes
test-cargo-miri/run-test.py Outdated Show resolved Hide resolved
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@ghost

This comment has been minimized.

Sign in to view
@ghost ghost closed this Jan 19, 2021
@ghost ghost reopened this Jan 19, 2021
@ghost

This comment has been minimized.

Sign in to view
@ghost ghost closed this Jan 19, 2021
@ghost ghost reopened this Jan 19, 2021
@ghost

This comment has been minimized.

Sign in to view
@RalfJung
Copy link
Member

Maybe GHA has network trouble... try waiting a few hours.

RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

@hyd-dev @teryror just as a heads-up, your two PRs will likely conflict heavily with each other, and whoever goes last will need to resolve those conflicts. cargo-miri is usually not changing much, but now we have two large-ish PRs working on it at the same time... not much we can do about that I think.

RalfJung
RalfJung reviewed Jan 23, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
@ghost ghost requested a review from RalfJung January 23, 2021 18:16
RalfJung
RalfJung reviewed Jan 24, 2021
View reviewed changes
cargo-miri/bin.rs Outdated Show resolved Hide resolved
@RalfJung
Copy link
Member

This looks great. :) I just have a minor nit left for the error. Can you squash the commits together a bit? Then I'll merge. :)

@RalfJung
Copy link
Member

Thank you so much @hyd-dev for digging into this and exploring various approaches, patiently re-implementing everything until you arrived at a solution that I am happy with. :-)

@bors r+

@bors
Copy link
Contributor

bors commented Jan 24, 2021

📌 Commit 2857792 has been approved by RalfJung

@bors
Copy link
Contributor

bors commented Jan 24, 2021

⌛ Testing commit 2857792 with merge 853254f...

@ghost
Copy link
Author

ghost commented Jan 24, 2021

Thanks, @RalfJung. Also I guess @teryror will need to resolve many merge conflicts... Sorry about that.

@bors
Copy link
Contributor

bors commented Jan 24, 2021

☀️ Test successful - checks-actions
Approved by: RalfJung
Pushing 853254f to master...

@bors bors merged commit 853254f into rust-lang:master Jan 24, 2021
@bors bors mentioned this pull request Jan 24, 2021
Closed
@ghost ghost deleted the proc-macro-unit-test branch January 24, 2021 16:36
@ghost
Copy link
Author

ghost commented Jan 24, 2021

Just realized I squashed one commit wrongly. eae9569 does not contain the doc comment (#1675 (comment)). Sorry.

@RalfJung
Copy link
Member

As long as the PR overall contains the comment, it's not a big deal.

@ghost ghost mentioned this pull request Feb 14, 2021
Merged
@dtolnay dtolnay mentioned this pull request May 5, 2022
Open
github-actions bot pushed a commit that referenced this pull request Sep 12, 2023
@matthiaskrgr
Rollup merge of #115761 - rustbot:docs-update, r=ehuss
21d1f53 
Update books

## rust-lang/edition-guide

1 commits in 2751bdcef125468ea2ee006c11992cd1405aebe5..34fca48ed284525b2f124bf93c51af36d6685492
2023-09-06 20:34:00 UTC to 2023-09-06 20:34:00 UTC

- Update Rust 2018 "Path and module system changes" for Rust 1.72 (rust-lang/edition-guide#285)

## rust-lang/nomicon

2 commits in 388750b081c0893c275044d37203f97709e058ba..e3f3af69dce71cd37a785bccb7e58449197d940c
2023-09-11 15:57:05 UTC to 2023-09-11 15:55:35 UTC

- specify which integer overflows we mean (rust-lang/nomicon#419)
- remove 'fail to call destructors' from okay-list (rust-lang/nomicon#420)

## rust-lang/reference

4 commits in d43038932adeb16ada80e206d4c073d851298101..ee7c676fd6e287459cb407337652412c990686c0
2023-09-09 20:08:06 UTC to 2023-08-16 16:59:33 UTC

- Specify bit validity and padding of some types (rust-lang/reference#1392)
- implementations.md typo fix (rust-lang/reference#1399)
- Update section on default layout for `repr(Rust)` (rust-lang/reference#1396)
- conditional-compilation.md: Mention the "none" target_os value (rust-lang/reference#1395)

## rust-lang/rust-by-example

4 commits in 07e0df2f006e59d171c6bf3cafa9d61dbeb520d8..c954202c1e1720cba5628f99543cc01188c7d6fc
2023-08-22 18:49:29 UTC to 2023-08-22 18:46:56 UTC

- Improve transparency of 5_i32 versus 5i32 (rust-lang/rust-by-example#1707)
- Removed redundant comma (rust-lang/rust-by-example#1735)
- Fixed link to Functions (rust-lang/rust-by-example#1734)
- Pedantic `'static` lifetime corrections (rust-lang/rust-by-example#1732)

## rust-lang/rustc-dev-guide

25 commits in b123ab4754127d822ffb38349ce0fbf561f1b2fd..08bb147d51e815b96e8db7ba4cf870f201c11ff8
2023-09-11 10:36:36 UTC to 2023-08-18 21:13:31 UTC

- make link more pleasant to eye too (rust-lang/rustc-dev-guide#1778)
- The current playground link used in the page of MIR shows a optimized… (rust-lang/rustc-dev-guide#1789)
- Add section about building an optimized version of `rustc` (rust-lang/rustc-dev-guide#1787)
- Set max line length in `.editorconfig` to 100 (rust-lang/rustc-dev-guide#1788)
- Update minor how-to-build-and-run.md spelling mistake (rust-lang/rustc-dev-guide#1785)
- add sections in 'using git' (#1675) (rust-lang/rustc-dev-guide#1784)
- link std-dev-guide from landing page (#1699) (rust-lang/rustc-dev-guide#1783)
- Reword sentence about using `./x` over `./x.py` (rust-lang/rustc-dev-guide#1782)
- remove (excessive) indentation (rust-lang/rustc-dev-guide#1781)
- coverage tests have moved, twice (rust-lang/rustc-dev-guide#1780)
- remove extraneous word (rust-lang/rustc-dev-guide#1779)
- llvm updates (rust-lang/rustc-dev-guide#1761)
- make link more pleasant to eye (rust-lang/rustc-dev-guide#1777)
- date-check: test suites/classes using "revisions" (rust-lang/rustc-dev-guide#1738)
- share link target (rust-lang/rustc-dev-guide#1740)
- indicate full hierarchy of config option (rust-lang/rustc-dev-guide#1776)
- remove stray word (rust-lang/rustc-dev-guide#1773)
- it is lower-case (rust-lang/rustc-dev-guide#1772)
- Suggest enabling patch-binaries-for-nix in `shell.nix` (rust-lang/rustc-dev-guide#1774)
- Add additional licensing concerns to docs (rust-lang/rustc-dev-guide#1775)
- Fix broken MD link format (rust-lang/rustc-dev-guide#1771)
- update internal terminology: Substs -> GenericArgs (rust-lang/rustc-dev-guide#1769)
- Update suggested.md : missing word (rust-lang/rustc-dev-guide#1770)
- Update outdated doc for types (rust-lang/rustc-dev-guide#1768)
- Add dropck documentation (rust-lang/rustc-dev-guide#1767)
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

cargo-miri builds proc-macro crate's unit tests as an executable and tries to interpret it as JSON
2 participants
@RalfJung @bors