Add crate build test for thumb* targets. [IRR-2018-embedded]
#53190
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- thumbv6m-none-eabi (Bare Cortex-M0, M0+, M1) - thumbv7em-none-eabi (Bare Cortex-M4, M7) - thumbv7em-none-eabihf (Bare Cortex-M4F, M7F, FPU, hardfloat) - thumbv7m-none-eabi (Bare Cortex-M3)
|
r? @aturon (rust_highfive has picked a reviewer for you, use r? to override) |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Question: Update: My guess is no need for guard, windows build uses |
jamesmunns
reviewed
Aug 9, 2018
| builder.ensure(compile::Test { compiler, target }); | ||
| } | ||
|
|
||
| if builder.no_std(target) == Some(true) { |
There was a problem hiding this comment.
Should this block be combined with the block above (line 980)?
There was a problem hiding this comment.
At first, I've written it in the form the second if is merged with the first if.
Later, I divided them into two ifs with clear intention. They are representing the two different purposes/concerns.
Let me explain the intention(s) below.
The first if
if builder.no_std(target) == Some(true) {
// the `test` doesn't compile for no-std targets
builder.ensure(compile::Std { compiler, target });
} else {
builder.ensure(compile::Test { compiler, target });
}
The first if serves for building compile::Test. Unfortunately, you can't build compile::Test for no_std. In that case, build compile::Std instead. It should be in the form of A or B. Adding unrelated C in one arm is not a good idea.
It's also a verbatim copy of @japaric's code found in dist.rs and supposed to be like a idiom which would be used in various places consistently. In the current bootstrap code, test support for no_std target is quite minimum. To widen the supported coverage for no_std, we might want to grep builder.ensure(compile::Test and replace it with this snippet.
But in future, no_std might get libtest support. At that time the whole if must be reverted back to the original single line:
builder.ensure(compile::Test { compiler, target });
If you merged two ifs, the whole change would be much less trivial.
The second if
if builder.no_std(target) == Some(true) {
// for no_std run-make (e.g. thumb*),
// we need a host compiler which is called by cargo.
builder.ensure(compile::Std { compiler, target: compiler.host });
}
The second if serves for a totally different purpose. For the majority of run-make tests, host compiler is NEVER needed. It is needed because we must run cargo and some dependent crate (e.g. cc) requires host compiler to be successfully built.
jamesmunns
reviewed
Aug 9, 2018
| # $ ./x.py clean | ||
| # $ ./x.py test --target thumbv6m-none-eabi,thumbv7m-none-eabi src/test/run-make | ||
|
|
||
| # Supported targets: |
There was a problem hiding this comment.
Yes, we now build required artifacts in ./x.py test execution.
jamesmunns
reviewed
Aug 9, 2018
| mkdir -p $(WORK_DIR) | ||
| -cd $(WORK_DIR) && rm -rf $(CRATE) | ||
| cd $(WORK_DIR) && $(CARGO) clone $(CRATE) --vers $(CRATE_VER) | ||
| cd $(WORK_DIR) && cd $(CRATE) && $(CARGO) build -j 1 --target $(TARGET) -v |
There was a problem hiding this comment.
@sekineh is the -j1 necessary here? I don't think it will be a problem for the cortex-m crate, but if we add more complex crates to these tests (or someone copy and pastes this working example), this could negatively affect CI build times.
There was a problem hiding this comment.
During debug this (-j 1) made log clearer. But I don't think it is needed any more. I removed this.
|
@jamesmunns and @kennytm |
|
self-review: Better to use the latter folder:
|
kennytm
reviewed
Aug 11, 2018
| ifneq (,$(filter $(TARGET),thumbv6m-none-eabi thumbv7em-none-eabi thumbv7em-none-eabihf thumbv7m-none-eabi)) | ||
|
|
||
| # We need to be outside of 'src' dir in order to run cargo | ||
| WORK_DIR := $(RUST_TEST_TMPDIR)/run-make/$(TARGET) |
There was a problem hiding this comment.
We typically use $(TMPDIR) in a run-make test.
| env | ||
| mkdir -p $(WORK_DIR) | ||
| -cd $(WORK_DIR) && rm -rf $(CRATE) | ||
| cd $(WORK_DIR) && $(CARGO) clone $(CRATE) --vers $(CRATE_VER) |
There was a problem hiding this comment.
cargo clone is not an internal command of cargo (rust-lang/cargo#1545), you'll need to install cargo-clone on the CI to get it. And I don't think it is a good idea have an un-vendored in the test case.
Perhaps consider making cortex-m a submodule?
The typical way to test an external crate is via cargotest:
./x.py test src/tools/cargotestThe problem is that cargotest is only designed for running on self-hosted platforms. If you have time, you could modify cargotest to run in cross-platform:
- In
src/tools/cargotest/main.rs- Edit
TEST_REPOSand specify the expected target triples of each repository - Edit
fn main()to accept the target - Edit
fn run_cargo_test()to pass--targetto cargo
- Edit
- Edit
struct Cargotestinsrc/bootstrap/test.rsto make it runnable whenhost ≠ target - Edit
src/ci/docker/dist-various-1/Dockerfileto testsrc/tools/cargotestfor the$RUN_MAKE_TARGETS.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
Fixed:[00:04:05] tidy error: /checkout/src/test/run-make/git_clone_sha1.sh:9: line longer than 100 chars Investigating:[00:04:05] tidy error: /checkout/src/test/run-make/git_clone_sha1.sh: incorrect license
|
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
You need copy those boilerplate MIT license texts you found in another |
|
Issues resolved. |
kennytm
approved these changes
Aug 16, 2018
|
@jamesmunns do you think it is good to go? @bors delegate=jamesmunns |
|
✌️ @jamesmunns can now approve this pull request |
jamesmunns
approved these changes
Aug 16, 2018
|
📌 Commit ad78c2f has been approved by |
|
⌛ Testing commit ad78c2f with merge 29330624ef32a0a0aeae8820a8eef44538ee2ef9... |
|
💔 Test failed - status-travis |
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
@bors retry GitHub archive issue.
|
bors
added a commit
that referenced
this pull request
Aug 17, 2018
Add crate build test for `thumb*` targets. [IRR-2018-embedded] ## Summary This PR adds `run-make` test that compiles `cortex-m` crate for all supported `thumb*-none-*` targets using `cargo` and stage2 `rustc`. - Supported `thumb*-none-*` targets: - thumbv6m-none-eabi (Bare Cortex-M0, M0+, M1) - thumbv7em-none-eabi (Bare Cortex-M4, M7) - thumbv7em-none-eabihf (Bare Cortex-M4F, M7F, FPU, hardfloat) - thumbv7m-none-eabi (Bare Cortex-M3) ## How to run & Example output I tested locally and all targets succeeded like below: ``` ./x.py clean ./x.py test --target thumbv6m-none-eabi,thumbv7em-none-eabi,thumbv7em-none-eabihf,thumbv7m-none-eabi src/test/run-make ``` ``` Check compiletest suite=run-make mode=run-make (x86_64-unknown-linux-gnu -> thumbv6m-none-eabi) running 5 tests ..... test result: ok. 5 passed; 0 failed; 0 ignored; 0 measured; 0 filtered out ``` ## How to re-run Remove `stamp` file for the test run. ``` rm build/x86_64-unknown-linux-gnu/test/run-make/thumb-none-cortex-m/stamp ``` Then run `test` ``` ./x.py test --target thumbv6m-none-eabi,thumbv7em-none-eabi,thumbv7em-none-eabihf,thumbv7m-none-eabi src/test/run-make (snip) running 5 tests iiii. test result: ok. 1 passed; 0 failed; 4 ignored; 0 measured; 0 filtered out ``` ## Artifacts You can examine the artifacts under the directory below: ``` sekineh@sekineh-VirtualBox:~/rustme10$ ls -l build/x86_64-unknown-linux-gnu/test/run-make/thumb-none-cortex-m/thumb-none-cortex-m/ total 4 drwxrwxr-x 7 sekineh sekineh 4096 8月 14 22:40 cortex-m ``` where `build/x86_64-unknown-linux-gnu/test/run-make/thumb-none-cortex-m/thumb-none-cortex-m/` is came from TMPDIR variable. ## Other notes For `test.rs` modification, I used the same logic as: - https://github.com/rust-lang/rust/blame/d8b3c830fbcdd14d085209a8dcc3399151f3286a/src/bootstrap/dist.rs#L652-L657 ``` if builder.no_std(target) == Some(true) { // the `test` doesn't compile for no-std targets builder.ensure(compile::Std { compiler, target }); } else { builder.ensure(compile::Test { compiler, target }); } ``` It is a useful snippet when adding `no_std` support to `src/bootstrap` code. CC @kennytm @jamesmunns @nerdyvaishali
|
☀️ Test successful - status-appveyor, status-travis |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Summary
This PR adds
run-maketest that compilescortex-mcrate for all supportedthumb*-none-*targets usingcargoand stage2rustc.thumb*-none-*targets:How to run & Example output
I tested locally and all targets succeeded like below:
How to re-run
Remove
stampfile for the test run.Then run
testArtifacts
You can examine the artifacts under the directory below:
where
build/x86_64-unknown-linux-gnu/test/run-make/thumb-none-cortex-m/thumb-none-cortex-m/is came from TMPDIR variable.Other notes
For
test.rsmodification, I used the same logic as:It is a useful snippet when adding
no_stdsupport tosrc/bootstrapcode.CC @kennytm @jamesmunns @nerdyvaishali