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.

Sign up for GitHub

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

Jump to bottom

Deduplicate platform lists in BuildIntegration #82395

Merged
merged 7 commits into from
Feb 23, 2023
Merged

Deduplicate platform lists in BuildIntegration #82395

merged 7 commits into from
Feb 23, 2023

Conversation

am11
Copy link
Member

@am11 am11 commented Feb 20, 2023

We only need to derive OS name from RuntimeIdentifier once.

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Feb 20, 2023
@ghost
Copy link

ghost commented Feb 20, 2023

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

Issue Details

We only need to derive OS name from RuntimeIdentifier once.

Author: am11
Assignees: -
Labels:

community-contribution, area-NativeAOT-coreclr
Milestone: -

@am11 am11 force-pushed the feature/native/target-props branch 8 times, most recently from 0005e74 to bc5a1aa Compare February 20, 2023 16:36
@runfoapp runfoapp bot mentioned this pull request Feb 20, 2023
Closed
@am11 am11 force-pushed the feature/native/target-props branch from bc5a1aa to bc3422f Compare February 20, 2023 20:17
@am11 am11 force-pushed the feature/native/target-props branch from bc3422f to 2f6287c Compare February 20, 2023 21:58
@am11 am11 marked this pull request as ready for review February 21, 2023 00:44
@am11
Copy link
Member Author

am11 commented Feb 21, 2023

linux_musl_arm64 sigsegv failure is unrelated; #76905.

@am11 am11 requested a review from jkotas February 22, 2023 01:17
@am11 am11 force-pushed the feature/native/target-props branch from dffe092 to f82b4c3 Compare February 22, 2023 04:20
@am11 am11 force-pushed the feature/native/target-props branch from f82b4c3 to 29a11f4 Compare February 22, 2023 06:17
@build-analysis build-analysis bot mentioned this pull request Feb 22, 2023
Closed
jkotas
jkotas reviewed Feb 22, 2023
View reviewed changes
src/coreclr/tools/Common/CommandLineHelpers.cs Outdated
};
}
"linux" or "linux-musl" => TargetOS.Linux,
_ when lowerToken.StartsWith("win") => TargetOS.Windows,
Copy link
Member

@jkotas jkotas Feb 22, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

It would be better if we accepted exact matches only. Does anything depend on the fuzziness of StartsWith matches here?

Copy link
Member Author

Choose a reason for hiding this comment

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

In the current revision, targets shouldn't pass win10 etc. (which was needed in earlier revision when we were passing values without using StartsWith in the targets).

jkotas
jkotas reviewed Feb 22, 2023
View reviewed changes
src/coreclr/nativeaot/BuildIntegration/Microsoft.DotNet.ILCompiler.targets
@@ -14,5 +14,5 @@ Copyright (c) .NET Foundation. All rights reserved.
<PropertyGroup>
<NeedNativePublishSupportForSDK6>$(NETCoreSdkVersion.StartsWith('6'))</NeedNativePublishSupportForSDK6>
</PropertyGroup>
<Import Project="$(ILCompilerTargetsPath)" Condition="'$(NeedNativePublishSupportForSDK6)' == 'true'"/>
<Import Project="$(ILCompilerTargetsPath)" Condition="'$(NeedNativePublishSupportForSDK6)' == 'true'"/>
Copy link
Member

Choose a reason for hiding this comment

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

NeedNativePublishSupportForSDK6 can be deleted (separate PR). We do not need to keep it around anymore.

jkotas
jkotas approved these changes Feb 22, 2023
View reviewed changes
Copy link
Member

@jkotas jkotas left a comment

Choose a reason for hiding this comment

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

LGTM. Thank you!

@build-analysis build-analysis bot mentioned this pull request Feb 23, 2023
Closed
@jkotas jkotas merged commit b969271 into dotnet:main Feb 23, 2023
@ghost ghost locked as resolved and limited conversation to collaborators Mar 25, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@jkotas jkotas jkotas approved these changes

@agocke agocke agocke left review comments

@MichalStrehovsky MichalStrehovsky Awaiting requested review from MichalStrehovsky MichalStrehovsky is a code owner

Assignees
No one assigned
Labels
area-NativeAOT-coreclr community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@am11 @agocke @jkotas