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

Exclude runtimeidentifiers for runtimepack #22658

Conversation

dsplaisted
Copy link
Member

Allow RuntimeIdentifiers to be excluded from runtime packs.

Will fix #19891, once we add RuntimePackExcludedRuntimeIdentifiers="android" to the ASP.NET KnownFrameworkReference items.

Also fixes an unrelated issue where ResolveAppHosts would generate errors in debug mode because it was generating messages prefixed with NETSDK error codes.

Copy link
Member

@jonathanpeppers jonathanpeppers left a comment

Choose a reason for hiding this comment

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

Otherwise, the test looks like this should fix #19891.

Comment on lines 99 to 100
HashSet<string> availableRids = new HashSet<string>(availableRuntimeIdentifiers);
HashSet<string> excludedRids = runtimeIdentifiersToExclude switch { null => null, _ => new HashSet<string>(runtimeIdentifiersToExclude) };
Copy link
Member

Choose a reason for hiding this comment

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

Is it worth using the ctor for HashSet<T> where you pass a StringComparer? StringComparer.Ordinal might help performance a bit? (Or OrdinalIgnoreCase if it is correct)

{
return candidateRuntimeIdentifier;
// Don't treat this as a match
return null;
}
}

// No compatible RID found in availableRuntimeIdentifiers
Copy link
Member

Choose a reason for hiding this comment

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

Looks like this comment is out of date now?

{
var testProject = new TestProject()
{
TargetFrameworks = "net6.0",
Copy link
Member

Choose a reason for hiding this comment

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

nit: don't we have a current framework field now to make it easier to update test TFMs?

@dsplaisted dsplaisted force-pushed the exclude-runtimeidentifiers-for-runtimepack branch from 2dc6156 to 840292f Compare December 1, 2021 18:35
@dsplaisted dsplaisted enabled auto-merge December 1, 2021 18:44
@dsplaisted
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

@dsplaisted
Copy link
Member Author

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 1 pipeline(s).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants