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

fix(ci): fix windows ci #9261

Closed
wants to merge 4 commits into from
Closed

Conversation

kt3k
Copy link
Member

@kt3k kt3k commented Jan 25, 2021

An alternative to #9254

This PR tries to avoid windows CI error by passing --py2 option to wpt util script. ref: https://github.com/web-platform-tests/wpt/blob/61d85c8/wpt#L10

@lucacasonato lucacasonato mentioned this pull request Jan 25, 2021
}
}
// Give the wpt runner time to start up.
std::thread::sleep(std::time::Duration::from_secs(5));
Copy link
Member

Choose a reason for hiding this comment

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

:)

@@ -5316,39 +5313,8 @@ fn web_platform_tests() {
.spawn()
.unwrap();

let stderr = proc.stderr.as_mut().unwrap();
let mut stderr = BufReader::new(stderr).lines();
Copy link
Member Author

Choose a reason for hiding this comment

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

We should remove use declaration of this as well?

_ => "python3",
};

eprintln!("If the wpt server fails or gets stuck, please set up your /etc/hosts file like specified in //docs/contributing/building_from_source.md.");
eprintln!("On windows python.exe needs to available in your path, and needs to point to python 3. On macOS and Linux, python3 needs to be available in your path.");
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should check this here in Rust? Like start python.exe --version and see that it has the correct output?

Copy link
Member

Choose a reason for hiding this comment

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

Would it be ok if I do this when I move the WPT runner to TS?

Copy link
Member

Choose a reason for hiding this comment

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

Not convinced about moving WPT runner to TS, but sure

@bartlomieju
Copy link
Member

There are some clippy warnings popping up:

warning: unused import: `std::io::BufReader`
 --> cli/tests/integration_tests.rs:8:5
  |
8 | use std::io::BufReader;
  |     ^^^^^^^^^^^^^^^^^^
  |
  = note: `#[warn(unused_imports)]` on by default

warning: variable does not need to be mutable
    --> cli/tests/integration_tests.rs:5308:7
     |
5308 |   let mut proc = Command::new(python)
     |       ----^^^^
     |       |
     |       help: remove this `mut`
     |
     = note: `#[warn(unused_mut)]` on by default

warning: 2 warnings emitted

@lucacasonato
Copy link
Member

I have reverted #9046 because I am going to do a full rewrite of the WPT runner. Because of this, this does not apply anymore.

@kt3k kt3k deleted the fix/win-wpt-script branch March 1, 2022 16:56
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants