Skip to content

Add crate build test for thumb* targets. [IRR-2018-embedded] #53190

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

Merged
merged 15 commits into from
Aug 17, 2018
Merged

Add crate build test for thumb* targets. [IRR-2018-embedded] #53190

Show file tree
Hide file tree
Changes from 9 commits
Commits
Show all changes
15 commits
Select commit Hold shift + click to select a range
d71cc75
[CI] run-make: experimental thumb test
sekineh Aug 6, 2018
320b240
[CI] run-make/thumb: add -vv to `cargo build` for diag
sekineh Aug 6, 2018
9e2dc55
[CI] run-make/thumb: add log for `cargo build`
sekineh Aug 6, 2018
5af644c
[CI] run-make/thumb: Add LD_LIBRARY_PATH experiment, etc.
sekineh Aug 6, 2018
633832f
[CI] run-make/thumb: 1st working version with hacky workaround.
sekineh Aug 7, 2018
7c438d4
[CI] run-make/thumb: remove hacky workaround.
sekineh Aug 8, 2018
bbbe441
[CI] run-make/thumb: tidy up variables
sekineh Aug 8, 2018
6c52653
[CI] run-make/thumb: support 4 thumb*-none-eabi* targets:
sekineh Aug 8, 2018
09854b0
[CI] run-make/thumb: remove a trailing space.
sekineh Aug 8, 2018
c2b612a
[CI] run-make/thumb: remove `-j 1` from cargo run
sekineh Aug 10, 2018
5c654f2
use a simple script instead of `cargo clone`.
sekineh Aug 14, 2018
62b3935
use TMPDIR instead of custom directory.
sekineh Aug 14, 2018
10395f3
change variable definition order (basic thing first).
sekineh Aug 14, 2018
e9c2de8
shorten the url to fix tidy error.
sekineh Aug 14, 2018
ad78c2f
add copyright from template
sekineh Aug 14, 2018
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion src/bootstrap/test.rs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -975,9 +975,19 @@ impl Step for Compiletest {
builder.ensure(compile::Rustc { compiler, target });
}

if builder.no_std(target) != Some(true) {
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 });
}

if builder.no_std(target) == Some(true) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should this block be combined with the block above (line 980)?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

// 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 });
}

builder.ensure(native::TestHelpers { target });
builder.ensure(RemoteCopyLibs { compiler, target });

Expand Down
35 changes: 35 additions & 0 deletions src/test/run-make/thumb-none-cortex-m/Makefile
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,35 @@
-include ../../run-make-fulldeps/tools.mk

# How to run this
# $ ./x.py clean
# $ ./x.py test --target thumbv6m-none-eabi,thumbv7m-none-eabi src/test/run-make

# Supported targets:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎊

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, we now build required artifacts in ./x.py test execution.

# - 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)

# See https://stackoverflow.com/questions/7656425/makefile-ifeq-logical-or
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)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We typically use $(TMPDIR) in a run-make test.


CRATE := cortex-m
CRATE_VER := 0.5.0

RUSTC := $(RUSTC_ORIGINAL)
LD_LIBRARY_PATH := $(HOST_RPATH_DIR)

all:
env
mkdir -p $(WORK_DIR)
-cd $(WORK_DIR) && rm -rf $(CRATE)
cd $(WORK_DIR) && $(CARGO) clone $(CRATE) --vers $(CRATE_VER)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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/cargotest

The 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:

  1. In src/tools/cargotest/main.rs
    1. Edit TEST_REPOS and specify the expected target triples of each repository
    2. Edit fn main() to accept the target
    3. Edit fn run_cargo_test() to pass --target to cargo
  2. Edit struct Cargotest in src/bootstrap/test.rs to make it runnable when host ≠ target
  3. Edit src/ci/docker/dist-various-1/Dockerfile to test src/tools/cargotest for the $RUN_MAKE_TARGETS.

cd $(WORK_DIR) && cd $(CRATE) && $(CARGO) build -j 1 --target $(TARGET) -v
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

During debug this (-j 1) made log clearer. But I don't think it is needed any more. I removed this.

else

all:

endif