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

Partial revert of #71486 to unblock dotnet/sdk #80791

Merged
merged 1 commit into from
Jan 18, 2023

Conversation

jkotas
Copy link
Member

@jkotas jkotas commented Jan 18, 2023

No description provided.

@ghost
Copy link

ghost commented Jan 18, 2023

Tagging subscribers to this area: @agocke, @MichalStrehovsky, @jkotas
See info in area-owners.md if you want to be subscribed.

Issue Details

null

Author: jkotas
Assignees: jkotas
Labels:

area-NativeAOT-coreclr

Milestone: -

@jkotas
Copy link
Member Author

jkotas commented Jan 18, 2023

dotnet/sdk#29800 (comment) has details

@jkotas
Copy link
Member Author

jkotas commented Jan 18, 2023

This is partial revert of #71486. It broke dotnet/sdk.

cc @janvorli @sec

@ViktorHofer
Copy link
Member

Thanks for the quick fix. When revisiting this, we should probably also stop setting TargetOS to$(OS) which in msbuild either returns Windows, OSX or Unix and isn't really helpful here. Also, TargetOS is expected to be a lowercase value.

@jkotas
Copy link
Member Author

jkotas commented Jan 18, 2023

Yes, let's have discussion about this in #80794 . cc @am11

We should take the revert first to get the dotnet/sdk unblocked with confidence. It has been stuck for 2 weeks.

@am11
Copy link
Member

am11 commented Jan 18, 2023

Yes, now FreeBSD is also listed:

if(string.IsNullOrEmpty(token))
{
if (RuntimeInformation.IsOSPlatform(OSPlatform.Windows))
return TargetOS.Windows;
else if (RuntimeInformation.IsOSPlatform(OSPlatform.Linux))
return TargetOS.Linux;
else if (RuntimeInformation.IsOSPlatform(OSPlatform.OSX))
return TargetOS.OSX;
else if (RuntimeInformation.IsOSPlatform(OSPlatform.FreeBSD))
return TargetOS.FreeBSD;
throw new NotImplementedException();
}
so it should work there without setting targetos=runtimeos.

When revisiting this, we should probably also stop setting TargetOS to$(OS) which in msbuild either returns Windows, OSX or Unix and isn't really helpful here. Also, TargetOS is expected to be a lowercase value.

#80794 is addressing this, so we can take both. :)

@jkotas jkotas merged commit c79777e into dotnet:main Jan 18, 2023
mdh1418 pushed a commit to mdh1418/runtime that referenced this pull request Jan 24, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Feb 17, 2023
@jkotas jkotas deleted the fix-sdk-blocker branch February 27, 2023 19:57
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants