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

bugfix(command/build.rs): Cancel Escape #396

Merged
merged 17 commits into from
Dec 28, 2018

Conversation

huangjj27
Copy link
Member

@huangjj27 huangjj27 commented Oct 5, 2018

fix #390

Make sure these boxes are checked! 📦✅

  • You have the latest version of rustfmt installed and have your
    cloned directory set to nightly
$ rustup override set nightly
$ rustup component add rustfmt-preview --toolchain nightly
  • You ran rustfmt on the code base before submitting
  • You reference which issue is being closed in the PR text

✨✨ 😄 Thanks so much for contributing to wasm-pack! 😄 ✨✨

@ashleygwilliams ashleygwilliams added this to the 0.6.0 milestone Oct 5, 2018
Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

so PathBuf doesn't have a default display on purpose- you'll likely want to use to_str or to_string_lossy (the previous can fail, the former is likely the better option here, though may sometimes print weird things).

@fitzgen
Copy link
Member

fitzgen commented Oct 5, 2018

so PathBuf doesn't have a default display on purpose- you'll likely want to use to_str or to_string_lossy (the previous can fail, the former is likely the better option here, though may sometimes print weird things).

I believe this is the preferred method for formatting paths: https://doc.rust-lang.org/nightly/std/path/struct.Path.html#method.display

@ashleygwilliams
Copy link
Member

oh huh, good call @fitzgen i didn't even know that existed. i'm curious that if that exists why Display trait isn't formally impl'd but anyways, seems the right way to go (as best as i can tell that display function does the same as to_string_lossy but it's hard to actually say)

src/command/build.rs Outdated Show resolved Hide resolved
@huangjj27
Copy link
Member Author

this fix may conflict with #389

@ashleygwilliams
Copy link
Member

@huangjj27 thanks for this! one ask- could you please include test(s)?

@ashleygwilliams ashleygwilliams added the needs tests please add tests to this PR label Oct 8, 2018
@huangjj27
Copy link
Member Author

huangjj27 commented Oct 8, 2018

@ashleygwilliams I would be glad to! but what kinds of tests should I add? Is it OK to just add a unit test to check the modified function? I tried to write a test, but failed figure out how to get the log when it builds successfully, in order to make some assertions on the log's content. Would you give me some tips?

@csmoe
Copy link
Member

csmoe commented Oct 8, 2018

@huangjj27 add a test case here to check whether windows path output is unneccesarily escaped as this PR intended to fix. the existing testcases are good examples for you to create a test case such as pkg_path_output_well_formatted or any name you like.

@ashleygwilliams
Copy link
Member

@huangjj27 if you would prefer i can also grab this PR and add the tests for you! just let me know what you'd like :)

@huangjj27 huangjj27 changed the title bugfix(command/build.rs): Cancel Escape WIP: bugfix(command/build.rs): Cancel Escape Oct 11, 2018
@huangjj27
Copy link
Member Author

huangjj27 commented Oct 11, 2018

Hi! I added a test for the change, but the test case seems to keep failing on installing wasm-bindgen, but the source code of test crate generated can be built using normal call of wasm-pack build, is that caused by something special in command::run_wasm_pack?

what the "source code of test crate generated" means the file generated by utils::fixture::js_hello_world()

here is the log:

Oct 11 22:10:22.496 ERRO Process exited with exit code: 1: wasm-bindgen failed to execute properly. stderr:

error: failed to read `target/wasm32-unknown-unknown/release/js_hello_world.wasm`
	caused by: 系统找不到指定的路径。 (os error 3)    #  (system can't find the file)

this error is likely caused by not finding the CARGO_MANIFEST_DIR variable in fixture::new, so the target path point to else where.

@huangjj27
Copy link
Member Author

@ashleygwilliams thanks! I'm working on the test(s) and glad to do the job, but I need some hints and times to finish it (for I could use only about an hour at night during workdays)

@ashleygwilliams
Copy link
Member

hrm. let me investigate! thanks @huangjj27 and no worries on taking your time, i just wanted to make sure you weren't stuck!

@huangjj27 huangjj27 changed the title WIP: bugfix(command/build.rs): Cancel Escape bugfix(command/build.rs): Cancel Escape Oct 13, 2018
@huangjj27
Copy link
Member Author

Hi, @fitzgen @ashleygwilliams , I just update a test case that can pass without redirecting $CARGO_TARGET_DIR of the temporary proejct. this can be merged after solving #408

@huangjj27
Copy link
Member Author

It seems the test case triggered an panic
, which is not found at local

@huangjj27
Copy link
Member Author

It's so weird that I passed the test on my windows but failed on appveyor. I even can't get the logs from appveyor. What should I do next?

@huangjj27
Copy link
Member Author

huangjj27 commented Oct 23, 2018

It's so strange that the result of cargo fmt is totally different. The code pushed is formatted in local, it gave me a total reversed result it turns out that I have used a older edition of the formator.

@huangjj27
Copy link
Member Author

---- build::it_format_out_dir_on_windows stdout ----
Created fixture at C:\projects\wasm-pack-071k0\target\t\.tmpReZwQh\wasm-pack
   [ERR]: There was a crate configuration error. Details:
thread 'build::it_format_out_dir_on_windows' panicked at 'directories in wasm-pack.log should be well formatted', tests\all\build.rs:56:5
stack backtrace:
failures:
    build::it_format_out_dir_on_windows
test result: FAILED. 34 passed; 1 failed; 0 ignored; 0 measured; 0 filtered out
error: test failed, to rerun pass '--test all'
Command exited with code 101

Hi, @ashleygwilliams , I get such error that never occur at local and I'm not sure if it's the appveyor that causes the problem. Would you re-trigger the CI?

@fitzgen
Copy link
Member

fitzgen commented Oct 30, 2018

I fixed the appveyor CI on master, so if you rebase, then it should start passing and we can merge this!

@fitzgen
Copy link
Member

fitzgen commented Oct 30, 2018

err, I suppose this hasn't had another round of review yet, so maybe not merge immediately after rebase :-p

@huangjj27
Copy link
Member Author

I once thought that it's using git bash in my vscode that makes the test failed. but after I change to powershell as integrated terminal, all test passed again.

in the CI, there is no user. begining with "C:\" is just enough
@huangjj27 huangjj27 changed the title WIP: bugfix(command/build.rs): Cancel Escape bugfix(command/build.rs): Cancel Escape Nov 17, 2018
@huangjj27
Copy link
Member Author

I finally figure out there is difference of the path between ch and local, and
I shouldn’t hard code much.

Copy link
Member

@ashleygwilliams ashleygwilliams left a comment

Choose a reason for hiding this comment

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

this is great! thanks so much for sticking with it!

@ashleygwilliams ashleygwilliams merged commit 9c01c38 into rustwasm:master Dec 28, 2018
@huangjj27 huangjj27 deleted the fix-390 branch December 29, 2018 08:54
@huangjj27 huangjj27 mentioned this pull request Jan 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Windows path output is unneccesarily escaped
5 participants