Make compiletest set cwd before running js tests #42059

richard-uk1 · 2017-05-17T15:21:18Z

Proposed fix for #38800.

Not all tests pass yet - I will mention failures here once the test suite has finished.

rust-highfive · 2017-05-17T15:21:26Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @aturon (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

richard-uk1 · 2017-05-17T15:22:45Z

r? @brson

richard-uk1 · 2017-05-17T16:11:06Z

Update: The following tests fail on run-pass:

    [run-pass] run-pass/asm-concat-src.rs
       (asm unsupported)
    [run-pass] run-pass/conditional-compile-arch.rs
       (no target_arch = "wasm32/64")
    [run-pass] run-pass/extern-call-deep.rs
    [run-pass] run-pass/issue-27859.rs
    [run-pass] run-pass/out-of-stack.rs
       (asm! is unsupported on this target)

aidanhs · 2017-05-18T00:04:56Z

Thanks for the PR @derekdreery! We'll check in now and again to make sure @brson or another reviewer gets to this soon.

carols10cents · 2017-05-22T16:45:58Z

ping @brson, welcome back from PTO! here's a PR for you to review! ❤️

alexcrichton · 2017-05-25T16:09:58Z

@derekdreery I'm forgetting why, but can you remind me why this is necessary to do on these tests (change the cwd)? Also, I wonder if we may wish to change the cwd for all tests instead of just emscripten/wasm ones?

richard-uk1 · 2017-05-28T10:43:52Z

The emscripten tools (node) expect the wasm file to be in the cwd, so tests don't work without setting the cwd to the directory of the file.

I wonder if we may wish to change the cwd for all tests instead of just emscripten/wasm ones?

I seem to recall that @brson was worried that this would cause test failures for other targets, but I may be mistaken.

alexcrichton · 2017-05-30T14:29:44Z

Ah ok, can we try setting it for all tests perhaps? It'd be great to avoid a bunch of "if-wasm-else-this" statements throughout

richard-uk1 · 2017-05-30T16:45:58Z

Ok I'll have a go. I'll let travis do the work rather than run it locally.

Mark-Simulacrum · 2017-06-04T00:42:25Z

I think we're almost there but a codegen test needs fixing:

[00:48:17] ---- [codegen] codegen/remap_path_prefix/main.rs stdout ----
[00:48:17] 	
[00:48:17] error: verification with 'FileCheck' failed
[00:48:17] status: exit code: 1
[00:48:17] command: /usr/lib/llvm-3.7/bin/FileCheck -input-file=/checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/remap_path_prefix/main.ll /checkout/src/test/codegen/remap_path_prefix/main.rs
[00:48:17] stdout:
[00:48:17] ------------------------------------------
[00:48:17] 
[00:48:17] ------------------------------------------
[00:48:17] stderr:
[00:48:17] ------------------------------------------
[00:48:17] /checkout/src/test/codegen/remap_path_prefix/main.rs:28:11: error: expected string not found in input
[00:48:17] // CHECK: !DIFile(filename: "/the/src/remap_path_prefix/main.rs", directory: "/the/cwd")
[00:48:17]           ^
[00:48:17] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/remap_path_prefix/main.ll:9:1: note: scanning from here
[00:48:17] @__rustc_debug_gdb_scripts_section__ = internal unnamed_addr constant [34 x i8] c"\01gdb_load_rust_pretty_printers.py\00", section ".debug_gdb_scripts", align 1
[00:48:17] ^
[00:48:17] /checkout/obj/build/x86_64-unknown-linux-gnu/test/codegen/remap_path_prefix/main.ll:44:6: note: possible intended match here
[00:48:17] !1 = !DIFile(filename: "/the/src/remap_path_prefix/main.rs", directory: "/the/cwd/build/x86_64-unknown-linux-gnu/test/codegen/remap_path_prefix")
[00:48:17]      ^
[00:48:17] 
[00:48:17] ------------------------------------------
[00:48:17] 
[00:48:17] thread '[codegen] codegen/remap_path_prefix/main.rs' panicked at 'explicit panic', /checkout/src/tools/compiletest/src/runtest.rs:2480
[00:48:17] note: Run with `RUST_BACKTRACE=1` for a backtrace.
[00:48:17] 
[00:48:17] 
[00:48:17] failures:
[00:48:17]     [codegen] codegen/remap_path_prefix/main.rs

arielb1 · 2017-06-06T11:47:33Z

@derekdreery

This looks ready except for the remap_path_prefix/main.rs test. Can you fix it so we can merge this?

malbarbo · 2017-06-13T19:03:54Z

src/librustc_back/target/wasm32_unknown_emscripten.rs

@@ -18,6 +18,8 @@ pub fn target() -> Result<Target, String> {
                          vec!["-s".to_string(),
                               "BINARYEN=1".to_string(),
                               "-s".to_string(),
+                               "BINARYEN_METHOD='native-wasm,interpret-binary'".to_string(),


interpret-binary is not necessary any more, node 8.0 can run wasm. See #42631

ooh cool I'll update

Edit: By default, emscripten provides its own node version 4. Running the tests would require the user to have an up to date version of node installed. I can do it on my computer but any CI scripts would have to make sure they have the correct version of node available, and that it was first in PATH.

richard-uk1 · 2017-06-14T11:04:58Z

@arielb1 I'm not familiar with the LLVM instructions in that test - can you give me some pointers?

EDIT: I understand what debuginfo is trying to do now, but not how this case should be fixed. The choices are

Disable this test
Disable this test for emscripten and make only emscripten change cwd
Don't change directory for this test
Change the directory in CHECK: DIFile to match (need to replace target with actual arch)

Can anyone advise?

alexcrichton · 2017-06-14T14:15:05Z

I think that's because the cwd of rustc itself is changed, right? Perhaps the cwd could only be changed for the test process and not the rustc process?

richard-uk1 · 2017-06-16T13:11:48Z

std::process is used to spawn the test commands. The current_dir method on the builder is used to set the cwd. I assume this means that the cwd is set just for that command, not the container process.

alexcrichton · 2017-06-16T15:00:02Z

Indeed yeah! And for the commands just for the target process, not for rustc itself.

alexcrichton · 2017-06-22T14:38:21Z

ping @derekdreery, just wanted to keep this on your radar!

richard-uk1 · 2017-06-22T18:36:58Z

Hi I'm just unsure of how to proceed.

…

On 22 Jun 2017 15:39, "Alex Crichton" ***@***.***> wrote: ping @derekdreery <https://github.com/derekdreery>, just wanted to keep this on your radar! — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42059 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU-XncG5rxkNN6JVktAgczKH6s0sLNbks5sGnyLgaJpZM4NeBzW> .

alexcrichton · 2017-06-23T15:22:40Z

@derekdreery does my comment above make sense? Only setting the cwd for the test process, not the rustc one?

shepmaster · 2017-06-30T20:51:09Z

@derekdreery just checking in to make sure you saw @alexcrichton's earlier comment. Please let us know if there's more information we can provide to help.

richard-uk1 · 2017-07-01T17:17:17Z

Hi sorry for the lead time. I've been away from my computer. Will endeavor to fix this this week.

…

On 30 Jun 2017 21:51, "Jake Goulding" ***@***.***> wrote: @derekdreery <https://github.com/derekdreery> just checking in to make sure you saw @alexcrichton <https://github.com/alexcrichton>'s earlier comment <#42059 (comment)>. Please let us know if there's more information we can provide to help. — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#42059 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/ABU-XiUmoqdQgUI486yoRK1UQ-YDw3aCks5sJV_RgaJpZM4NeBzW> .

arielb1 · 2017-07-11T10:57:28Z

Hi @derekdreery - are you still looking on this PR? Just a friendly ping to make sure this isn't getting lost.

richard-uk1 · 2017-07-11T11:48:02Z

Apologies for the time. I made this PR because I've used rust quite a lot and want to understand/contribute to the internals a bit more. I'm finding doing this PR very helpful for getting used to the structure of rust-lang/rust, but it means my work is quite slow (not helped by having to earn money by doing paid work etc. :P )

richard-uk1 · 2017-07-11T23:05:32Z

OK so I've done some more work on this. Since node 8 supports wasm native (pointed out by @malbarbo), I used my own node rather than emsdk's. With version 8, it seems the cwd requirement has gone away! I'm able to run (most) of the run-pass tests fine (without my code changes). However, none of the run-fail tests work, claiming that their wasm files are missing (they are missing - I checked).

So I suspect that this PR will become redundant, but then there is the question: What is the best way to set up testing in CI? Is it to just override the node version used? And if so how do I get the run-fail tests working?

What do people think of this?

alexcrichton · 2017-07-12T15:26:27Z

@derekdreery oh the CI testing is all done through docker images in src/ci/docker, so it's just whatever we configure there. Do you have example for failures with run-fail tests?

richard-uk1 · 2017-07-17T16:06:36Z

I've changed compilertest to only change the working directory for the test process, and tests seem to run correctly (including stage 0/1 i.e. x64 -> x64 builds). Unfortunately, I'm using a custom script to remove the node path added by emscripten, so I can get native wasm in my node (v8.1.4):

#!/usr/bin/python
import os

REMOVE_STRING = ":/home/rdodd/packages/emsdk-portable/node/4.1.1_64bit/bin"

def main():
    old_path = os.environ["PATH"]
    new_path = old_path.replace(REMOVE_STRING, "")
    print(new_path)

if __name__ == '__main__':
    main()

I could add some code to compiletest to do this (actually splitting the path, not using this hacky string replace), but it feels like it should be in the docker shell script. I'm going to let the automated tests run to see if this is indeed an issue, and then have a think about how to solve it.

richard-uk1 · 2017-07-17T16:07:13Z

We could use emulated wasm on node v4, but it would be an order of magnitude slower

alexcrichton · 2017-07-17T20:20:40Z

@derekdreery thanks! Do you think this is ready to merge? Or because you need a local workaround do you think we should hold off?

richard-uk1 · 2017-07-18T09:48:52Z

I think yes it can be merged, and the workaround can be added in a separate PR, if you want to run the test suite against emscripten target in the future. That test would fail currently as there are some things (e.g. inline asm) that won't work with emscripten, and those tests need to be ignored for this target.~

I'm confused as to why some mac targets are failing.

aidanhs · 2017-07-18T14:16:17Z

The logs were AWOL and those targets would be skipped anyway given it was just a PR build, so I restarted the jobs and now they show as fine.

alexcrichton · 2017-07-18T18:53:58Z

I think with this implemented we should be able to remove this workaround, right? Could this PR also delete those shim node.sh scripts and ensure that the container still passes tests?

alexcrichton · 2017-07-22T16:12:57Z

r? @alexcrichton

arielb1 · 2017-07-25T23:12:35Z

I think with this implemented we should be able to remove this workaround, right? Could this PR also delete those shim node.sh scripts and ensure that the container still passes tests?

@derekdreery - @alexcrichton thinks you can remove the workaround. Just a friendly ping to keep this on your radar.

richard-uk1 · 2017-07-26T08:31:24Z

Hi yeah sorry didn't realise you were waiting on me. I'll have a look.

richard-uk1 · 2017-07-26T08:44:28Z

I've removed the workaround you mention, and put node in the PATH.

alexcrichton · 2017-07-26T16:57:02Z

@bors: r+

Thanks!

bors · 2017-07-26T16:57:03Z

📌 Commit 874ecdc has been approved by alexcrichton

bors · 2017-07-26T18:27:25Z

⌛ Testing commit 874ecdc with merge d02fb3b...

…xcrichton Make compiletest set cwd before running js tests Proposed fix for #38800. Not all tests pass yet - I will mention failures here once the test suite has finished.

bors · 2017-07-26T21:08:35Z

☀️ Test successful - status-appveyor, status-travis
Approved by: alexcrichton
Pushing d02fb3b to master...

rust-highfive assigned aturon May 17, 2017

rust-highfive assigned brson and unassigned aturon May 17, 2017

aidanhs added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label May 18, 2017

Mark-Simulacrum 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 May 28, 2017

richard-uk1 force-pushed the bugfix/fix_emscripten_tests branch from ff7050b to 91f236d Compare May 30, 2017 16:52

malbarbo mentioned this pull request Jun 13, 2017

Add a travis builder for wasm32-unknown-emscripten #42631

Merged

malbarbo reviewed Jun 13, 2017

View reviewed changes

richard-uk1 added 3 commits July 17, 2017 17:01

Make compiletest set cwd before running js tests

f4e329f

Try running all tests using cwd

c35030a

Only set cwd for test process, not compiler

5aa8cc8

richard-uk1 force-pushed the bugfix/fix_emscripten_tests branch from 91f236d to 5aa8cc8 Compare July 17, 2017 16:01

rust-highfive assigned alexcrichton and unassigned brson Jul 22, 2017

Remove custom node script

874ecdc

bors merged commit 874ecdc into rust-lang:master Jul 26, 2017

richard-uk1 deleted the bugfix/fix_emscripten_tests branch July 27, 2017 07:10

Make compiletest set cwd before running js tests #42059

Make compiletest set cwd before running js tests #42059

Conversation

richard-uk1 commented May 17, 2017

rust-highfive commented May 17, 2017

richard-uk1 commented May 17, 2017

richard-uk1 commented May 17, 2017 • edited Loading

aidanhs commented May 18, 2017

carols10cents commented May 22, 2017

alexcrichton commented May 25, 2017

richard-uk1 commented May 28, 2017

alexcrichton commented May 30, 2017

richard-uk1 commented May 30, 2017

Mark-Simulacrum commented Jun 4, 2017

arielb1 commented Jun 6, 2017

malbarbo Jun 13, 2017

Choose a reason for hiding this comment

richard-uk1 Jun 14, 2017 • edited Loading

Choose a reason for hiding this comment

richard-uk1 commented Jun 14, 2017 • edited Loading

alexcrichton commented Jun 14, 2017

richard-uk1 commented Jun 16, 2017 • edited Loading

alexcrichton commented Jun 16, 2017

alexcrichton commented Jun 22, 2017

richard-uk1 commented Jun 22, 2017 via email

alexcrichton commented Jun 23, 2017

shepmaster commented Jun 30, 2017

richard-uk1 commented Jul 1, 2017 via email

arielb1 commented Jul 11, 2017

richard-uk1 commented Jul 11, 2017

richard-uk1 commented Jul 11, 2017

alexcrichton commented Jul 12, 2017

richard-uk1 commented Jul 17, 2017

richard-uk1 commented Jul 17, 2017

alexcrichton commented Jul 17, 2017

richard-uk1 commented Jul 18, 2017

aidanhs commented Jul 18, 2017 • edited Loading

alexcrichton commented Jul 18, 2017

alexcrichton commented Jul 22, 2017

arielb1 commented Jul 25, 2017

richard-uk1 commented Jul 26, 2017

richard-uk1 commented Jul 26, 2017

alexcrichton commented Jul 26, 2017

bors commented Jul 26, 2017

bors commented Jul 26, 2017

bors commented Jul 26, 2017

richard-uk1 commented May 17, 2017 •

edited

Loading

richard-uk1 Jun 14, 2017 •

edited

Loading

richard-uk1 commented Jun 14, 2017 •

edited

Loading

richard-uk1 commented Jun 16, 2017 •

edited

Loading

aidanhs commented Jul 18, 2017 •

edited

Loading