Skip to content

Conversation

@jkotas
Copy link
Member

@jkotas jkotas commented Jan 18, 2023

No description provided.

@ghost ghost assigned jkotas Jan 18, 2023
@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 jkotas requested review from LakshanF and sbomer January 18, 2023 16:47
@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.

@jkotas jkotas requested a review from ViktorHofer January 18, 2023 17:12
@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