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 init-compiler.sh for older clang #8219

Merged

Conversation

jakubch1
Copy link
Member

Latest changes made by PR #8189
broke our build:

clang-5.0 : error : invalid linker name in argument '-fuse-ld=lld' [/__w/1/s/src/Microsoft.VisualStudio.Coverage.InstrumentationMethod/Microsoft.VisualStudio.Coverage.InstrumentationMethod.proj]
##[error]clang-5.0(0,0): error : (NETCORE_ENGINEERING_TELEMETRY=Build) invalid linker name in argument '-fuse-ld=lld'

probably the reason is that we use older clang. If I correctly understand that we can check if clang is at least 9 before even checking for this -fuse-id.

@jakubch1
Copy link
Member Author

@omajid could you please check those changes?

@premun
Copy link
Member

premun commented Nov 25, 2021

This will probably unblock one of the issues in dotnet/runtime#61668?

dotnet/runtime#61668 (comment)

@omajid
Copy link
Member

omajid commented Nov 25, 2021

I am testing it out now to see if this still works for me.

cc @am11 @janvorli

@janvorli
Copy link
Member

Isn't the issue actually caused by the fact that we output the error message and our CI seeing an output with "error" thinks there was a failure? Wouldn't we get the same message in case the lld was not installed even with newer clang versions? I wonder if we would also need to redirect the output to /dev/null.

@am11
Copy link
Member

am11 commented Nov 25, 2021

Isn't the issue actually caused by the fact that we output the error message and our CI seeing an output with "error" thinks there was a failure?

Yes, this is bizarre. Only reason to redirect to stdin was so we can see the message in logs, but /dev/null is fine since something in the pipeline is not liking the text error:..

@janvorli, just curious shouldn't we be using clang 9 at minimum in all CI legs since .NET Core 3?

@jakubch1
Copy link
Member Author

@janvorli redirecting to /dev/null worked for our pipeline (without changing ifs order). I can add it to PR also. WDYT?

@am11
Copy link
Member

am11 commented Nov 25, 2021

Your ordering looks fine too. We don't need two separate conditions for compiler and its version.

Co-authored-by: Adeel Mujahid <3840695+am11@users.noreply.github.com>
@janvorli
Copy link
Member

I can add it to PR also. WDYT?

Yes, please.

@jakubch1
Copy link
Member Author

Isn't the issue actually caused by the fact that we output the error message and our CI seeing an output with "error" thinks there was a failure?

Yes, this is bizarre. Only reason to redirect to stdin was so we can see the message in logs, but /dev/null is fine since something in the pipeline is not liking the text error:..

@janvorli, just curious shouldn't we be using clang 9 at minimum in all CI legs since .NET Core 3?

Our pipeline is code coverage related. It's not part of dotnet/runtime repository.

@janvorli
Copy link
Member

shouldn't we be using clang 9 at minimum in all CI legs since .NET Core 3?

Clang 5.0 is the one available on Alpine <= 3.9. Based on https://docs.microsoft.com/en-us/dotnet/core/install/linux, we support Alpine >= 3.8 on .NET 3.1.

@premun
Copy link
Member

premun commented Nov 26, 2021

Are we happy like this, should we merge?

@jakubch1
Copy link
Member Author

I think we can. Conditions are merged and error output is going to /dev/null

@premun
Copy link
Member

premun commented Nov 26, 2021

Ok, merging to unblock runtime asap. If there are some objections from @janvorli or @am11 we can address them in later

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