Skip to content

Partial fix for #38489. #38601

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

Merged
merged 2 commits into from
Dec 31, 2016
Merged

Partial fix for #38489. #38601

merged 2 commits into from
Dec 31, 2016

Conversation

schulzch
Copy link
Contributor

Fixes script name resolution for windows by invoking emcc.bat instead of emcc, etc.

Remaining issue:

Traceback (most recent call last):
  File "C:\Program Files\Emscripten\emscripten\1.35.0\\emcc", line 1309, in <module>
    final = shared.Building.llvm_opt(final, link_opts, DEFAULT_FINAL)
  File "C:\Program Files\Emscripten\emscripten\1.35.0\tools\shared.py", line 1471, in llvm_opt
    assert os.path.exists(target), 'Failed to run llvm optimizations: ' + output
AssertionError: Failed to run llvm optimizations:

@rust-highfive
Copy link
Contributor

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @eddyb (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.

@eddyb
Copy link
Member

eddyb commented Dec 25, 2016

r? @brson

@rust-highfive rust-highfive assigned brson and unassigned eddyb Dec 25, 2016
name.to_string()
}

#[cfg(windows)]
Copy link
Member

Choose a reason for hiding this comment

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

Instead of #[cfg], could this use cfg! perhaps to get type checking everywhere? Something like:

pub fn cmd(name: &str) -> String {
    if cfg!(windows) {
        format!("{}.bat", name)
    } else {
        name.to_string()
    }
}

@brson
Copy link
Contributor

brson commented Dec 30, 2016

Thank you so much for helping with emscripten support @schulzch !

I think this is the right thing to do for now, but I think it's also incorrect because it makes the emscripten target spec conditional on the host platform, which I do not think is appropriate. I'm not sure the best solution.

@bors r+

@bors
Copy link
Collaborator

bors commented Dec 30, 2016

📌 Commit 14994ac has been approved by brson

@bors
Copy link
Collaborator

bors commented Dec 31, 2016

⌛ Testing commit 14994ac with merge b68d329...

bors added a commit that referenced this pull request Dec 31, 2016
Partial fix for #38489.

Fixes script name resolution for windows by invoking `emcc.bat` instead of `emcc`, etc.

Remaining issue:
```
Traceback (most recent call last):
  File "C:\Program Files\Emscripten\emscripten\1.35.0\\emcc", line 1309, in <module>
    final = shared.Building.llvm_opt(final, link_opts, DEFAULT_FINAL)
  File "C:\Program Files\Emscripten\emscripten\1.35.0\tools\shared.py", line 1471, in llvm_opt
    assert os.path.exists(target), 'Failed to run llvm optimizations: ' + output
AssertionError: Failed to run llvm optimizations:
```
@bors
Copy link
Collaborator

bors commented Dec 31, 2016

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

@bors bors merged commit 14994ac into rust-lang:master Dec 31, 2016
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.

6 participants