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.

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

run-make/thumb-none-cortex-m is no-op due to incompatible # only-target directives #67018

Closed
pnkfelix opened this issue Dec 4, 2019 · 3 comments · Fixed by #68194
Closed
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems

Comments

@pnkfelix
Copy link
Member

pnkfelix commented Dec 4, 2019

I was working on making a run-make test today for a particular thumb target.

While looking over how we do this elsewhere, I found this bit of code:

# only-thumbv6m-none-eabi
# only-thumbv7em-none-eabi
# only-thumbv7em-none-eabihf
# only-thumbv7m-none-eabi

From my local testing, it seems like having multiple # only-target lines of this form has the net effect of making the test a no-op that does not test anything: since a target matching line 13 will not match line 14, and vice versa, there are no targets that can run on this test.

We should either revert the portion of the commit that did this:

f15d20c#diff-463654a579bb644f5d94cd0e1a6ff232

- # 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))
+ # only-thumbv6m-none-eabi
+ # only-thumbv7em-none-eabi
+ # only-thumbv7em-none-eabihf
+ # only-thumbv7m-none-eabi

or figure out some other way to express the desired property here

(or change the semantics of # only-target when multiple targets are provided to make it gather all such targets up in a set before doing the check for membership... but I am wary of making such a change since doing it right would require differentiating e.g. processor target vs OS target, for example...)

@jonas-schievink jonas-schievink added A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. WG-embedded Working group: Embedded systems T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) labels Dec 4, 2019
@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 4, 2019

Also, It looks like there may be other instances of the same mistake elsewhere in the test suite, such as in:

# only-thumbv7m-none-eabi
# only-thumbv6m-none-eabi

@jonas-schievink
Copy link
Contributor

Oof, not good. It shouldn't be hard to make rustbuild outright reject this scenario though.

@pnkfelix
Copy link
Member Author

pnkfelix commented Dec 5, 2019

Further effort on #67020 has led me to conclude that these # only-<target-triple> lines do not work at all, at least not for cross-compilation.

@jonas-schievink jonas-schievink added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Dec 10, 2019
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 15, 2020
…xcrichton

Fix CI for embedded ARM targets

Closes rust-lang#67018

It would be better to move the `thumb-none-cortex-m` test into the `cargotest` suite, but it doesn't seem to support cross-compilation.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 15, 2020
…xcrichton

Fix CI for embedded ARM targets

Closes rust-lang#67018

It would be better to move the `thumb-none-cortex-m` test into the `cargotest` suite, but it doesn't seem to support cross-compilation.
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this issue Jan 17, 2020
…xcrichton

Fix CI for embedded ARM targets

Closes rust-lang#67018

It would be better to move the `thumb-none-cortex-m` test into the `cargotest` suite, but it doesn't seem to support cross-compilation.
@bors bors closed this as completed in f5abfb1 Jan 22, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-testsuite Area: The testsuite used to check the correctness of rustc C-bug Category: This is a bug. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. WG-embedded Working group: Embedded systems
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants