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

Use SingleFileHost #11797

Merged
merged 4 commits into from
Jun 1, 2020
Merged

Use SingleFileHost #11797

merged 4 commits into from
Jun 1, 2020

Conversation

swaroop-sridhar
Copy link
Contributor

@swaroop-sridhar swaroop-sridhar commented May 28, 2020

This commit implements the following changes for single-file apps:

@swaroop-sridhar swaroop-sridhar force-pushed the host branch 2 times, most recently from 71ffa71 to 37e9d2f Compare May 29, 2020 00:30
This commit implements the following changes for single-file apps:
* When publishing self-contained single-file apps:
  * Use `SingleFileHost` instead of `apphost`
  * Trim the native components of the runtime published for the app.
   * Fixes dotnet#11567.

* Implements the [optional additional settings](https://github.com/dotnet/designs/blob/master/accepted/2020/single-file/design.md#optional-settings) for .net 5 `PublishSingleFile`.
  * dotnet/runtime#36590
@@ -2,18 +2,18 @@

<PropertyGroup>
<OutputType>Exe</OutputType>
<TargetFramework>netcoreapp3.0</TargetFramework>
<TargetFramework>netcoreapp5.0</TargetFramework>
Copy link
Member

Choose a reason for hiding this comment

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

Preferrably this should use net5.0 for the TargetFramework.

Comment on lines 174 to 178
IsSelfContained ?
IsSingleFile ? RuntimePackAssets.Where(item => !item.GetMetadata(MetadataKeys.DropFromSingleFile).Equals("true"))
.Select(item => RuntimePackAssetInfo.FromItem(item)) :
RuntimePackAssets.Select(item => RuntimePackAssetInfo.FromItem(item)) :
Enumerable.Empty<RuntimePackAssetInfo>();
Copy link
Member

Choose a reason for hiding this comment

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

Consider the following to avoid repeating the same Select clause:

Suggested change
IsSelfContained ?
IsSingleFile ? RuntimePackAssets.Where(item => !item.GetMetadata(MetadataKeys.DropFromSingleFile).Equals("true"))
.Select(item => RuntimePackAssetInfo.FromItem(item)) :
RuntimePackAssets.Select(item => RuntimePackAssetInfo.FromItem(item)) :
Enumerable.Empty<RuntimePackAssetInfo>();
IsSelfContained ?
RuntimePackAssets.Where(item => IsSingleFile ? !item.GetMetadata(MetadataKeys.DropFromSingleFile).Equals("true") : true)
.Select(item => RuntimePackAssetInfo.FromItem(item)) :
Enumerable.Empty<RuntimePackAssetInfo>();

Comment on lines +1018 to +1019
<_IsSingleFilePublish Condition="'$(PublishSingleFile)' == ''">false</_IsSingleFilePublish>
<_IsSingleFilePublish Condition="'$(PublishSingleFile)' != ''">$(PublishSingleFile)</_IsSingleFilePublish>
Copy link
Member

Choose a reason for hiding this comment

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

What's the reason for having a separate _IsSingleFilePublish property rather than using PublishSingleFile directly?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The PublishSingleFile property is captured by telemetry, which reports the three states: true, false, and unset. So, I wanted to leave the user input in its pure state and use a different variable.

@swaroop-sridhar
Copy link
Contributor Author

Thanks for the review, @dsplaisted. I have committed the changes you've suggested.

@swaroop-sridhar
Copy link
Contributor Author

The Linux/Osx failures are because singelfilehost was not packaged into the host packages for these platforms. These will get fixed once dotnet/runtime#37147 is available in the installer repo.

@dsplaisted
Copy link
Member

The Linux/Osx failures are because singelfilehost was not packaged into the host packages for these platforms. These will get fixed once dotnet/runtime#37147 is available in the installer repo.

So once that is fixed then you should be able to update stage 0 for this PR and merge it?

@swaroop-sridhar
Copy link
Contributor Author

So once that is fixed then you should be able to update stage 0 for this PR and merge it?

Yes that is my plan -- would like to have it in Preview 6.

  • The fix for singlefilehost is already checked in the runtime repo. I'm waiting for the changes to propagate to SDK->Installer repos, so that I can update stage0.
  • With a more recent SDK (5.0.100-preview.6.20278.11) there's a new unrelated failure on Windows. Looks like there's a bug in the crossgen2 package (missing clrcompression.dll). I'm working on getting that fixed.

(To include update from runtime)
Updating stage0 caused crossgen2 tests to fail because of missing dependencies in the corssgen2 package.
dotnet/runtime#37196

Therefore, try disabling the tests temporarily.
@swaroop-sridhar
Copy link
Contributor Author

@dsplaisted Updating stage0 triggered crossgen2 failure: dotnet/runtime#37196.
Therefore, I disabled two crossgen2 tests in this commit: d48f3e0

I think it is a good idea to disable these tests temporarily, rather than holding the global.json update (and this PR) until dotnet/runtime#37196 is fixed.
I wanted to call this out before checking in. Thanks.

CC: @trylek.

@dsplaisted
Copy link
Member

@dsplaisted Updating stage0 triggered crossgen2 failure: dotnet/runtime#37196.
Therefore, I disabled two crossgen2 tests in this commit: d48f3e0

I think it is a good idea to disable these tests temporarily, rather than holding the global.json update (and this PR) until dotnet/runtime#37196 is fixed.
I wanted to call this out before checking in. Thanks.

CC: @trylek.

Sounds good. We don't have a great process to track re-enabling tests like this. Will you remember to re-enable these? We could also file an issue in the sdk repo to track re-enabling the tests when the bug is fixed.

@swaroop-sridhar swaroop-sridhar merged commit 8288f55 into dotnet:master Jun 1, 2020
@swaroop-sridhar
Copy link
Contributor Author

OK @dsplaisted, I filed #11845 to re-enable the tests once the underlying issue is fixed.

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

Successfully merging this pull request may close these issues.

Use SingleFileHost for self-contained single-file apps
2 participants