Skip to content

Use linker extensibility to enable better trimming#23512

Merged
mkArtakMSFT merged 2 commits into
release/5.0-preview7from
prkrishn/more-trimming
Jul 1, 2020
Merged

Use linker extensibility to enable better trimming#23512
mkArtakMSFT merged 2 commits into
release/5.0-preview7from
prkrishn/more-trimming

Conversation

@pranavkm
Copy link
Copy Markdown
Contributor

  • Configure TrimmerDefaults=link if unspecified
  • Allow Microsoft.AspNetCore.* and Microsoft.Extensions.* packages to be trimmed.

* Configure TrimmerDefaults=link if unspecified
* Allow Microsoft.AspNetCore.* and Microsoft.Extensions.* packages to be trimmed.
@pranavkm pranavkm requested review from a team, eerhardt and sbomer June 30, 2020 14:42
@pranavkm pranavkm added this to the 5.0.0-preview7 milestone Jun 30, 2020
@pranavkm pranavkm added the area-blazor Includes: Blazor, Razor Components label Jun 30, 2020
using (var file = File.OpenRead(assemblyPath))
{
var peReader = new PEReader(file);
using var peReader = new PEReader(file);
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

👍

Comment thread src/Razor/test/testassets/blazorwasm/Program.cs

<ManagedAssemblyToLink
IsTrimmable="true"
Condition="'%(Extension)' == '.dll' AND ($([System.String]::Copy('%(Filename)').StartsWith('Microsoft.AspNetCore.')) or $([System.String]::Copy('%(Filename)').StartsWith('Microsoft.Extensions.')))" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

(minor) I think there is a way to not duplicate this condition, and instead just use @(_BlazorTypeGranularAssembly) to update these ManagedAssemblyToLink items.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Is that by removing and adding it? ItemGroup Update doesn't work inside of a target.

Copy link
Copy Markdown
Member

@eerhardt eerhardt left a comment

Choose a reason for hiding this comment

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

LGTM

@pranavkm
Copy link
Copy Markdown
Contributor Author

The test failure is because the SDK is very new and has a timestamp that's newer than current time PST. We could wait another hour and retry:

image

public override bool Execute()
{
using var fileStream = File.Create(TrimmerFile.ItemSpec);
var rootDescriptor = CreateRootDescriptorContents();
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Adding the extra package file uncovered an issue with publish incrementalism. We were always writing a new trimmer descriptor which would cause the linker to be non-incremental. The task was updated to use the same pattern as WriteLinesToFile.WriteOnlyWhenDifferent

@eerhardt
Copy link
Copy Markdown
Member

With this change, you should also be able to remove this line:

<!-- Workaround for https://github.com/mono/linker/issues/1298 -->
<assembly fullname="System.Security.Cryptography.X509Certificates" />

@pranavkm
Copy link
Copy Markdown
Contributor Author

Thanks @eerhardt. I'll do that as a follow up once I've tested it. We don't have any test coverage for trimmed projects in this branch.

@mkArtakMSFT mkArtakMSFT merged commit 8768cab into release/5.0-preview7 Jul 1, 2020
@mkArtakMSFT mkArtakMSFT deleted the prkrishn/more-trimming branch July 1, 2020 03:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area-blazor Includes: Blazor, Razor Components

Projects

None yet

Development

Successfully merging this pull request may close these issues.

6 participants