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

Fix src/test/run-make/issue-36710 on cross-compiled targets #103179

Merged
merged 7 commits into from
Nov 18, 2022
Merged

Fix src/test/run-make/issue-36710 on cross-compiled targets #103179

merged 7 commits into from
Nov 18, 2022

Conversation

pietroalbini
Copy link
Member

This PR fixes the src/test/run-make/issue-36710 test not working on cross-compiled targets by telling the make infra how to run tests remotely with remote-test-server.

This PR includes a revert of #102723 (cc @pcc), which disabled that test on all cross-compiled targets.

@rust-highfive
Copy link
Collaborator

r? @jyn514

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 18, 2022
jyn514
jyn514 reviewed Oct 18, 2022
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

Thanks! The code to support cross compiling seems reasonable, but the number of new ignores makes me worried you've missed a target and this will fail on the full test suite. I guess we can just try it and see though.

src/test/run-make/issue-36710/Makefile
# ignore-none no-std is not supported
# ignore-wasm32 FIXME: don't attempt to compile C++ to WASM
# ignore-wasm64 FIXME: don't attempt to compile C++ to WASM
# ignore-nvptx64-nvidia-cuda FIXME: can't find crate for `std`
Copy link
Member

Choose a reason for hiding this comment

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

Why is this a FIXME? How would CUDA ever get support for libstd? (Is there some way we can say "ignore all targets without std"?)

Copy link
Member Author

Choose a reason for hiding this comment

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

That was present before #102723 (which I reverted), I don't have context on why those are fixmes.

Copy link
Member

Choose a reason for hiding this comment

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

The commit message adding this originally says 1fa48cf

nvtptx64-nvidia-cuda fails in rustc saying it can't find std. The rust
platforms support page says that std is supported on cuda so this is
surprising.

sounds like something is broken here but no need to fix it in your PR. cc @RDambrosio016 in case you know who maintains this target.

src/test/run-make/issue-36710/Makefile Outdated Show resolved Hide resolved
src/test/run-make/issue-36710/Makefile Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Oct 18, 2022

@bors rollup=iffy

@jyn514
Copy link
Member

jyn514 commented Oct 18, 2022

@bors r+

@bors
Copy link
Contributor

bors commented Oct 18, 2022

📌 Commit ca362edbdde20d7592643142ea2f00e9f65e2a9e has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 18, 2022
@jyn514 jyn514 added the A-testsuite Area: The testsuite used to check the correctness of rustc label Oct 18, 2022
@pietroalbini
Copy link
Member Author

Bors seemed to have lost the r+.

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Oct 24, 2022

📌 Commit ca362edbdde20d7592643142ea2f00e9f65e2a9e has been approved by jyn514

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Oct 24, 2022

⌛ Testing commit ca362edbdde20d7592643142ea2f00e9f65e2a9e with merge eab05ae07aab4737b537dd0da052a7bcb2f21e99...

@bors
Copy link
Contributor

bors commented Oct 24, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 24, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue. label Oct 25, 2022
@pietroalbini
Copy link
Member Author

Installed the missing compiler in that builder, hopefully this is the only builder where it's missing 🤞

@jyn514
Copy link
Member

jyn514 commented Oct 27, 2022

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 27, 2022

📌 Commit cb096c148285f5f1146ec1a61cf0b297c78ec473 has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Oct 27, 2022
@bors
Copy link
Contributor

bors commented Oct 30, 2022

⌛ Testing commit cb096c148285f5f1146ec1a61cf0b297c78ec473 with merge fa8492c23ff5f8d57adf27c88dda77a1477623c8...

@bors
Copy link
Contributor

bors commented Oct 30, 2022

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Oct 30, 2022
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rustbot rustbot added the T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) label Nov 3, 2022
@pietroalbini
Copy link
Member Author

Android failed because of two problems:

  • When configuring the NDK we were not setting the cc and cxx bootstrap configs to point to the NDK's compilers. This didn't affect building code with Cargo, but resulted in the CC and CXX environment variables not being set correctly in run-make tests.
  • Android used the default value for TEST_DEVICE_ADDR and so didn't set the environment variable, breaking the detection logic I had in run-make.

I fixed both of the problems and successfully ran the run-make suite locally with src/ci/docker/run.sh.

@bors
Copy link
Contributor

bors commented Nov 14, 2022

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

@jyn514
Copy link
Member

jyn514 commented Nov 17, 2022

Sorry for the delay. r=me after a rebase.

@jyn514 jyn514 added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Nov 17, 2022
@pietroalbini
add support for running binaries with remote-test-client on run-make
b2c74ab
@pietroalbini
Revert "test: run-make: skip when cross-compiling"
ecaa342 
This reverts commit 0567fec.
@pietroalbini
issue-36710 test can now run on cross-compiled targets
c80af32
@pietroalbini
install the target g++ in armhf-gnu
09340e3
@pietroalbini
set correct default value for cc and cxx on android
6d81602
@pietroalbini
run tests on the remote device even when the default address is used
6bfbd11 
When running tests inside the Android emulator, bootstrap doesn't set
the TEST_DEVICE_ADDR environment variable, as the default address
(127.0.0.1:12345) is used.

Instead, REMOTE_TEST_CLIENT is set all the times when remote testing is
needed, and in no other cases. To ensure Android tests are executed in
the emulator, change the check.
@pietroalbini
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Nov 17, 2022

📌 Commit 6bfbd11 has been approved by jyn514

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2022

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 17, 2022
@pietroalbini
Copy link
Member Author

@bors r=jyn514

@bors
Copy link
Contributor

bors commented Nov 17, 2022

📌 Commit 00ec679 has been approved by jyn514

It is now in the queue for this repository.

@bors
Copy link
Contributor

bors commented Nov 17, 2022

🌲 The tree is currently closed for pull requests below priority 1. This pull request will be tested once the tree is reopened.

@bors
Copy link
Contributor

bors commented Nov 18, 2022

⌛ Testing commit 00ec679 with merge 30117a1...

@bors
Copy link
Contributor

bors commented Nov 18, 2022

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing 30117a1 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Nov 18, 2022
@bors bors merged commit 30117a1 into rust-lang:master Nov 18, 2022
@rustbot rustbot added this to the 1.67.0 milestone Nov 18, 2022
@pietroalbini pietroalbini deleted the pa-run-in-run-make branch November 18, 2022 10:16
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (30117a1): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

This benchmark run did not return any relevant results for this metric.

Max RSS (memory usage)

Results

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)		 - - 0
Regressions ❌
(secondary)		 3.7% [2.5%, 5.0%] 2
Improvements ✅
(primary)		 - - 0
Improvements ✅
(secondary)		 - - 0
All ❌✅ (primary) - - 0

Cycles

This benchmark run did not return any relevant results for this metric.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jyn514 jyn514 jyn514 left review comments

Assignees

@jyn514 jyn514

Labels
A-testsuite Area: The testsuite used to check the correctness of rustc merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) T-infra Relevant to the infrastructure team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.67.0
Development

Successfully merging this pull request may close these issues.
7 participants
@pietroalbini @rust-highfive @jyn514 @bors @rust-log-analyzer @rust-timer @rustbot