Skip to content
This repository has been archived by the owner on Dec 19, 2018. It is now read-only.

Cross-compile Windows Services hosting package #1189

Closed
wants to merge 1 commit into from
Closed

Cross-compile Windows Services hosting package #1189

wants to merge 1 commit into from

Conversation

henkmollema
Copy link
Contributor

@henkmollema henkmollema commented Aug 28, 2017

Cross-compile Microsoft.AspNetCore.Hosting.WindowsServices for .NET 4.6.1 and .NET Core 2.0.

I had the use the .NET Core MyGet feed as the ServiceBase API is only available in version 4.5.0-*. Perhaps the package in the ASP.NET Core MyGet feed can be updated as well?

Resolves #904

/cc @muratg @JunTaoLuo

@@ -13,5 +13,6 @@
<SerilogExtensionsLoggingVersion>1.4.0</SerilogExtensionsLoggingVersion>
<SerilogFileSinkVersion>3.2.0</SerilogFileSinkVersion>
<SystemReflectionMetadataVersion>1.5.0-*</SystemReflectionMetadataVersion>
<SystemServiceProcessVersion>4.5.0-*</SystemServiceProcessVersion>
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Should we specify the package name completely? E.g. SystemServiceProcessServiceControllerVersion. If this setting is desired/necessary at all.

Copy link
Contributor

Choose a reason for hiding this comment

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

Should we specify the package name completely?

Either way is fine. I'll be deleting this file soon anyways as we move to a system called "lineups" to manage our package versions.

Any reason you need 4.5.0-* specifically? Or will 4.4.0 work?

cref https://github.com/aspnet/Universe/blob/dev/build/dependencies.props#L114

Copy link
Contributor Author

@henkmollema henkmollema Aug 28, 2017

Choose a reason for hiding this comment

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

Yeah, I saw a PR of you coming by where you implemented that. Figured it would be fine.

I've used the 4.5.0 packages because the API was added in .NET Core 2.1: dotnet/corefx#22920. I thought this maps to 4.5.0-* packages of the runtime.

Copy link
Contributor

Choose a reason for hiding this comment

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

Seconded, it's easier if this can be 4.4.0, then we'll update to 4.5.0 at a later date in all repos at once.

Copy link
Contributor

Choose a reason for hiding this comment

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

The 4.4.0 package only has a few types from System.ServiceProcess.* namespace. 4.5.0 adds more. Which version we need will depends which API is actually referenced from hosting. I haven't looked closely at what we use. If we need 4.5.0 that's no problem, we just need to setup our internal build processes to start using new packages from the corefx team.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This package uses ServiceBase, which was added in .NET Core 2.1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Good to know. Then yes, we'll need the 4.5.0 version of the package.

@Tratcher Tratcher requested a review from natemcmaster August 28, 2017 16:41
NuGet.config Outdated
@@ -2,6 +2,7 @@
<configuration>
<packageSources>
<clear />
<add key="dotnet-core" value="https://dotnet.myget.org/F/dotnet-core/api/v3/index.json" />
Copy link
Member

Choose a reason for hiding this comment

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

We don't add these feeds directly, we mirror the packages to our feed. @ryanbrandenburg

Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, System.ServiceProcess.ServiceController is already being mirrored to aspnetcore-ci-dev.

@natemcmaster
Copy link
Contributor

Talked with @Tratcher. It sounds like .NET Core 2.0 doesn't have an implementation of this API yet. We haven't started building for .NET Core 2.1 yet, so this may need to wait.

@henkmollema
Copy link
Contributor Author

I see. Feel free to ping me if we target .NET Core 2.1 and you want me to update this PR. 😄

NuGet.config Outdated
@@ -2,6 +2,7 @@
<configuration>
<packageSources>
<clear />
<add key="dotnet-core" value="https://dotnet.myget.org/F/dotnet-core/api/v3/index.json" />
Copy link
Contributor

Choose a reason for hiding this comment

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

Yeah, System.ServiceProcess.ServiceController is already being mirrored to aspnetcore-ci-dev.

@@ -13,5 +13,6 @@
<SerilogExtensionsLoggingVersion>1.4.0</SerilogExtensionsLoggingVersion>
<SerilogFileSinkVersion>3.2.0</SerilogFileSinkVersion>
<SystemReflectionMetadataVersion>1.5.0-*</SystemReflectionMetadataVersion>
<SystemServiceProcessVersion>4.5.0-*</SystemServiceProcessVersion>
Copy link
Contributor

Choose a reason for hiding this comment

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

Seconded, it's easier if this can be 4.4.0, then we'll update to 4.5.0 at a later date in all repos at once.

@natemcmaster
Copy link
Contributor

natemcmaster commented Aug 28, 2017

Ok. I'm going to assign this PR to @JunTaoLuo for now so we can follow up on it later. We'll hold off merging till we can build against .NET Core 2.1.

Blocked on https://github.com/aspnet/Coherence-Signed/issues/657

@JunTaoLuo
Copy link
Contributor

We shouldn't be cross-compiling here. We should wait for 2.1 where the ServiceBase API is available. This was why I was asking about when we are taking the new runtime last week.

@henkmollema
Copy link
Contributor Author

henkmollema commented Aug 28, 2017

How would we get access to the ServiceBase API? Will it be part of .NET Standard 2.1?

@natemcmaster
Copy link
Contributor

.NET Standard 2.1

There are no plans for this AFAIK, however the API surface should be available when you target netcoreapp2.1

@henkmollema
Copy link
Contributor Author

henkmollema commented Aug 28, 2017

I see, but does that mean we drop .NET 4.6.1?

@JunTaoLuo
Copy link
Contributor

Ah I mis-spoke, I meant we will need use netcoreapp2.1, not netcoreapp2.0. We plan to have it available for both core and framework.

@henkmollema
Copy link
Contributor Author

That makes sense, figured we need that. I was already looking for netcoreapp2.1 but could not find it in the ASP.NET repos 😄

@Tratcher
Copy link
Member

We should also update the ServiceBase sample for the new Generic Host.

And add a sample for the current RunAsService.

@JunTaoLuo
Copy link
Contributor

FYI The current eta is for middle of November.

@ghost ghost removed the cla-already-signed label Nov 14, 2017
@henkmollema
Copy link
Contributor Author

@Tratcher I've updated with the new preview package of System.ServiceProcess.ServiceController. I've cross-compiled with .NET Standard 2.0 instead of .NET Core. Does that makes sense? Or should we target .NET Core instead.

@Tratcher
Copy link
Member

netstandard2.0 is preferred wherever possible.

@Tratcher Tratcher added 1 - Ready and removed blocked Blocked labels Nov 17, 2017
@Tratcher Tratcher added this to the 2.1.0 milestone Nov 17, 2017
Copy link
Contributor

@JunTaoLuo JunTaoLuo left a comment

Choose a reason for hiding this comment

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

The changes are correct and is ready to merge. However, there's a few infrastructure reactions I need to make (e.g. ingestion of system packages need to be marked in our Universe builds).

@JunTaoLuo
Copy link
Contributor

Merged in e03c6a7. Thanks @henkmollema for the contribution!

@JunTaoLuo JunTaoLuo closed this Nov 20, 2017
@henkmollema henkmollema deleted the crosscompile-windowsservices branch November 21, 2017 08:44
@henkmollema
Copy link
Contributor Author

🎉

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.

6 participants