Skip to content

Conversation

@juj
Copy link
Collaborator

@juj juj commented Mar 16, 2021

…nse_file

@juj juj force-pushed the fix_windows_emar_rsp_invocation branch from 320773f to dcdccf3 Compare March 16, 2021 10:36
Copy link
Collaborator

@sbc100 sbc100 left a comment

Choose a reason for hiding this comment

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

lgtm!

I've been trying to fix the windows difference upstream BTW: https://reviews.llvm.org/D80876

@sbc100
Copy link
Collaborator

sbc100 commented Mar 16, 2021

lgtm!

I've been trying to fix the windows difference upstream BTW: https://reviews.llvm.org/D80876

The problem I'm trying to fix upstream is that unlike llvm-ar and other tools, clang itself does not use windows style response files, even when running on windows. It only does that when its running as clang-cl (i.e. emulating cl.exe). So this is change might have some risks?

@juj
Copy link
Collaborator Author

juj commented Mar 16, 2021

So this is change might have some risks?

Hmm, maybe? I think we should go by our test suite on things like this, and have test cases that prove or disprove the risks, i.e. if there is some yet escaping scenario that this would break, we should unsurface it to another test.

I am not particularly bitten about this behavior in present, but was just looking at this when searching whether we have response file tests in the suite for #13673.

@juj juj merged commit 16d006c into emscripten-core:main Mar 16, 2021
@sbc100
Copy link
Collaborator

sbc100 commented Mar 16, 2021

So this is change might have some risks?

Hmm, maybe? I think we should go by our test suite on things like this, and have test cases that prove or disprove the risks, i.e. if there is some yet escaping scenario that this would break, we should unsurface it to another test.

Agreed.

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.

2 participants