Skip to content

Make --skip-unresolved the default when not running from SDK#90512

Merged
sbomer merged 7 commits intodotnet:mainfrom
Youssef1313:Youssef1313/skip-unresolved
Sep 1, 2023
Merged

Make --skip-unresolved the default when not running from SDK#90512
sbomer merged 7 commits intodotnet:mainfrom
Youssef1313:Youssef1313/skip-unresolved

Conversation

@Youssef1313
Copy link
Copy Markdown
Member

@Youssef1313 Youssef1313 commented Aug 14, 2023

Closes #90469

@ghost ghost added area-Tools-ILLink .NET linker development as well as trimming analyzers linkable-framework Issues associated with delivering a linker friendly framework community-contribution Indicates that the PR has been added by a community member labels Aug 14, 2023
@ghost
Copy link
Copy Markdown

ghost commented Aug 14, 2023

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

Issue Details

null

Author: Youssef1313
Assignees: -
Labels:

area-Tools-ILLink

Milestone: -

@vitek-karas
Copy link
Copy Markdown
Member

The test failures are real it seems - could you please take a look? It's likely that couple of tests took an unintentional dependency on the setting being false by default. It should be enough to add an explicit --skip-unresolved false to these tests.

@vitek-karas
Copy link
Copy Markdown
Member

@sbomer - what do you think about making this change. Personally I like it - as it makes the tests behave the "expected" way by default. The fact that if something is using the command line directly (even though unsupported) and gets a more predictable behavior is just a nice side effect for me.

@sbomer
Copy link
Copy Markdown
Member

sbomer commented Aug 16, 2023

Seems like a good direction to me - we want the command-line to be consistent with the MSBuild behavior. Please also remove this line:

<_ExtraTrimmerArgs>--skip-unresolved true $(_ExtraTrimmerArgs)</_ExtraTrimmerArgs>

@Youssef1313 Youssef1313 requested a review from sbomer as a code owner August 17, 2023 06:06
@sbomer
Copy link
Copy Markdown
Member

sbomer commented Sep 1, 2023

A downside of this change is that we will be losing some test coverage of the --skip-unresolved false mode (see #91007 (comment) for a discussion of where this might be useful). cc @mrvoorhe in case you have any concerns.

@sbomer sbomer merged commit 8d13e3e into dotnet:main Sep 1, 2023
@sbomer
Copy link
Copy Markdown
Member

sbomer commented Sep 1, 2023

Thanks for your contribution @Youssef1313!

@Youssef1313 Youssef1313 deleted the Youssef1313/skip-unresolved branch September 2, 2023 10:52
@mrvoorhe
Copy link
Copy Markdown
Contributor

mrvoorhe commented Sep 5, 2023

Thanks for the heads up @sbomer. I'm not sure if I'm concerned about test coverage or not. This code has been pretty stable without much in the way of test coverage. If I become concerned about it I can grab our old tests for unresolved stubbing and adapt them to be --skip-unresolved=false tests

@ghost ghost locked as resolved and limited conversation to collaborators Oct 5, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Labels

area-Tools-ILLink .NET linker development as well as trimming analyzers community-contribution Indicates that the PR has been added by a community member linkable-framework Issues associated with delivering a linker friendly framework

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ILLinker should special case facade assemblies

4 participants