Skip to content

Do not include redist files in runtime packs - #66805

Merged
VSadov merged 3 commits into
dotnet:mainfrom
VSadov:apiSets
Mar 19, 2022
Merged

Do not include redist files in runtime packs#66805
VSadov merged 3 commits into
dotnet:mainfrom
VSadov:apiSets

Conversation

@VSadov

@VSadov VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member

The goal is to not include with apps unnecessary files like api sets.

Once api set dlls like api-ms-win-core-whatever-l1-1-0.dll are not in the runtime pack, SDK will stop placing them into self-containing apps. (SDK just copies all that is in RuntimeFiles)

Fixes:#65758

@ghost ghost assigned VSadov Mar 18, 2022
@ghost

ghost commented Mar 18, 2022

Copy link
Copy Markdown

Tagging subscribers to this area: @dotnet/runtime-infrastructure
See info in area-owners.md if you want to be subscribed.

Issue Details

In progress.

The goal is to not include with apps unnecessary files like api sets.

Author: VSadov
Assignees: VSadov
Labels:

area-Infrastructure

Milestone: -

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

Note that this also stops copying ucrtbase.dll.

Local deployment of crt is a perf and security concern. We should be using the one that comes with the system and serviced by the updates.

Besides, on Win10+ the local ucrtbase is never used anyways. Even if a local one is newer, the system component always wins over.

@VSadov
VSadov marked this pull request as ready for review March 18, 2022 17:41
@VSadov
VSadov requested a review from jkoritzinsky March 18, 2022 17:42
@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

CC @jkotas

@jkotas

jkotas commented Mar 18, 2022

Copy link
Copy Markdown
Member

Are these files gone from the msi as well? (ie they won't get installed at C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 anymore)

@jkotas

jkotas commented Mar 18, 2022

Copy link
Copy Markdown
Member

Also, do CI tests still pass on Windows 7 with this change? You may want to trigger extra platforms or outer loop tests to validate it.

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

Are these files gone from the msi as well? (ie they won't get installed at C:\Program Files\dotnet\shared\Microsoft.NETCore.App\7.0.0 anymore)

I do not know where else they would pe picked from, if we stop placing them into RuntimeFiles, but it is worth checking to be sure.

@jkoritzinsky

Copy link
Copy Markdown
Member

/azp run runtime-extra-platforms

@azure-pipelines

Copy link
Copy Markdown
Azure Pipelines successfully started running 1 pipeline(s).

@hoyosjs

hoyosjs commented Mar 18, 2022

Copy link
Copy Markdown
Member

@VSadov if you also want to remove them from runtime packs (which will) and the platform manifest - it would be here

<PlatformManifestFileEntry Include="api-ms-win-core-console-l1-1-0.dll" IsNative="true" FallbackFileVersion="$(WindowsForwarderFileVersion)" />
. My only question is where would Win7 apps pick them up from?

@jkoritzinsky jkoritzinsky left a comment

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.

LGTM if the Windows 7 leg in runtime-extra-platforms passes.

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

I am not sure about the PlatformManifestFileEntry. It looks like we keep everything that was ever included there, so I did not remove the entries.

@jkoritzinsky

Copy link
Copy Markdown
Member

Generally, the platform manifest must contain every file we ever shipped in an out-of-band package that is now in box. Since these always shipped with Microsoft.NETCore.App, it's probably okay to remove them.

A good test to be sure would be to reference a package that targets netcoreapp1.x (before the SDK added implicit package references to Microsoft.NETCore.App) and do a self-contained build. If the output of this experiment doesn't include any .NET Core 1.x assets, then we should be safe to remove the entries.

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

Re: Windows7

The runtime (including native/managed components) has the following dependencies:

    KERNEL32.dll
    ADVAPI32.dll
    ole32.dll
    OLEAUT32.dll
    USER32.dll
    VERSION.dll
    SHELL32.dll
    api-ms-win-core-winrt-l1-1-0.dll
    api-ms-win-crt-stdio-l1-1-0.dll
    api-ms-win-crt-runtime-l1-1-0.dll
    api-ms-win-crt-string-l1-1-0.dll
    api-ms-win-crt-convert-l1-1-0.dll
    api-ms-win-crt-heap-l1-1-0.dll
    api-ms-win-crt-utility-l1-1-0.dll
    api-ms-win-crt-math-l1-1-0.dll
    api-ms-win-crt-time-l1-1-0.dll
    api-ms-win-crt-locale-l1-1-0.dll
    api-ms-win-crt-filesystem-l1-1-0.dll

These are basically dependencies of singlefilehost.exe.
Of which

  • api-ms-win-core-winrt-l1-1-0.dll is delay-loaded conditionally on presence of winrt, so it will not be used on Win7
  • crt stuff should use the sytem-wide ucrtbase.dll

Evidently not including all these files does not prevent singlefile apps working on Win7 SP1 (after #63196).

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

Re: omitting these files from Microsoft.NETCore.App

I have checked artifacts\bin\testhost\net7.0-windows-Release-x64\shared\Microsoft.NETCore.App\7.0.0 and the files are no longer included.

@jkoritzinsky - is that the right place to look at? (i.e. no files will be added later somehow)

@dotnet dotnet deleted a comment from azure-pipelines Bot Mar 18, 2022
@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

A good test to be sure would be to reference a package that targets netcoreapp1.x (before the SDK added implicit package references to Microsoft.NETCore.App) and do a self-contained build. If the output of this experiment doesn't include any .NET Core 1.x assets, then we should be safe to remove the entries.

Since this is a good-to-have clean up, but there is a chance of failure, I will do that as a separate change once this gets all the way to SDK and we see that all is ok.
Otherwise, I would not want to rollback the whole thing if cleanup causes troubles.

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

created an issue to track PlatformManifestFileEntry cleanup. - #66860

@VSadov

VSadov commented Mar 18, 2022

Copy link
Copy Markdown
Member Author

@jkoritzinsky - which of the legs are Windows7 ? Did we pass?

@jkoritzinsky

Copy link
Copy Markdown
Member

We need to watch the runtime-extra-platforms (Libaries Test Run release coreclr windows x86 Release) leg.

That leg runs the Windows 7 test run.

@hoyosjs

hoyosjs commented Mar 18, 2022

Copy link
Copy Markdown
Member

Helix is out today - we won't get results looks like.

@VSadov

VSadov commented Mar 19, 2022

Copy link
Copy Markdown
Member Author

The only failure is on OSX and seems unrelated (some ThreadPool test).
The remaining running legs are tvOS and wasm and take forever to finish, so I think we are good to merge.

@VSadov
VSadov merged commit 9c04c3a into dotnet:main Mar 19, 2022
@VSadov

VSadov commented Mar 19, 2022

Copy link
Copy Markdown
Member Author

Thanks!!

@VSadov
VSadov deleted the apiSets branch March 19, 2022 06:44
radekdoulik pushed a commit to radekdoulik/runtime that referenced this pull request Mar 30, 2022
* do not copy redist files to artefacts

* do not copy redist files to tests

* Do not include Redist files in runtime packs
@ghost ghost locked as resolved and limited conversation to collaborators Apr 18, 2022
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.

5 participants