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

travis: Get an emscripten builder online #39120

Merged
merged 1 commit into from
Jan 21, 2017

Conversation

alexcrichton
Copy link
Member

This commit adds a new entry to the Travis matrix which will execute emscripten
test suites. Along the way it updates a few bits of the test suite to continue
passing on emscripten, such as:

  • Ignoring i128/u128 tests as they're presumably just not working (didn't
    investigate as to why)
  • Disabling a few process tests (not working on emscripten)
  • Ignore some num tests in libstd (255u8.leading_zeros() is wrong on emscripten #39119)
  • Fix some warnings when compiling

@rust-highfive
Copy link
Collaborator

r? @brson

(rust_highfive has picked a reviewer for you, use r? to override)

@alexcrichton
Copy link
Member Author

@brson do you think now is the right time for this? Or should we perhaps wait on more solid support before it gets tier 2 status?

@brson
Copy link
Contributor

brson commented Jan 18, 2017

I suspect allowing the emscripten commit to drift will be a recipe for sadness, but I don't mind trying if we can get it to pass bors.

@brson
Copy link
Contributor

brson commented Jan 18, 2017

@bors r+

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit 759b4d2 has been approved by brson

@alexcrichton
Copy link
Member Author

@bors: r=brson

Oh hey looks like using a pinned version works!

@bors
Copy link
Contributor

bors commented Jan 18, 2017

📌 Commit aa2e7f5 has been approved by brson

This commit adds a new entry to the Travis matrix which will execute emscripten
test suites. Along the way it updates a few bits of the test suite to continue
passing on emscripten, such as:

* Ignoring i128/u128 tests as they're presumably just not working (didn't
  investigate as to why)
* Disabling a few process tests (not working on emscripten)
* Ignore some num tests in libstd (rust-lang#39119)
* Fix some warnings when compiling
@alexcrichton
Copy link
Member Author

@bors: r=brson

@bors
Copy link
Contributor

bors commented Jan 19, 2017

📌 Commit e8f9d2d has been approved by brson

alexcrichton added a commit to alexcrichton/rust that referenced this pull request Jan 20, 2017
travis: Get an emscripten builder online

This commit adds a new entry to the Travis matrix which will execute emscripten
test suites. Along the way it updates a few bits of the test suite to continue
passing on emscripten, such as:

* Ignoring i128/u128 tests as they're presumably just not working (didn't
  investigate as to why)
* Disabling a few process tests (not working on emscripten)
* Ignore some num tests in libstd (rust-lang#39119)
* Fix some warnings when compiling
@alexcrichton
Copy link
Member Author

alexcrichton commented Jan 20, 2017

@bors: r-

I forgot to add it to the matrix :\. Fixed in the rollup though which I hope to land today.

bors added a commit that referenced this pull request Jan 20, 2017
bors added a commit that referenced this pull request Jan 21, 2017
@bors bors merged commit e8f9d2d into rust-lang:master Jan 21, 2017
@alexcrichton alexcrichton deleted the emscripten-tests branch January 21, 2017 18:04
@@ -45,7 +45,7 @@ pub fn check(build: &mut Build) {
let target = path.join(cmd);
let mut cmd_alt = cmd.to_os_string();
cmd_alt.push(".exe");
if target.exists() ||
if target.is_file() ||
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any reasons behind the mixed use of is_file() and exists() ?

Also, I don't quite understand what target.join(cmd_alt) is for; when we have, say, cmd=="python", path=="C:\\Python27", and thus cmd_alt=="python\.exe", are we not checking for C:\\Python27\python\python\.exe which I doubt make sense? I haven't tested it on any Windows PC, though.

Copy link
Member Author

Choose a reason for hiding this comment

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

Nah they're likely mostly historical, this was changed as I wanted it to not pick up directories

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.

5 participants