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

Consume transport pack from dotnet/runtime #1936

Merged
merged 5 commits into from
Aug 17, 2021

Conversation

ViktorHofer
Copy link
Member

dotnet/runtime packages don't contain reference assemblies anymore. As
windowsdesktop distributes some of dotnet/runtime's reference assemblies
in their targeting pack, consuming a transport package from
dotnet/runtime which contains all the required assets (ref + lib + docs)

Removing stale properties and subscriptions as well.

cc @ericstj @Anipik

dotnet/runtime packages don't contain reference assemblies anymore. As
windowsdesktop distributes some of dotnet/runtime's reference assemblies
in their targeting pack, consuming a transport package from
dotnet/runtime which contains all the required assets (ref + lib + docs)

Removing stale properties and subscriptions as well.
@ViktorHofer ViktorHofer self-assigned this Aug 13, 2021
@dreddy-work
Copy link
Member

Name of the transport package is little confusing to me. Should it be sounded like Microsoft.RuntimeForWindowsDesktop.Internal.Transport instead of Microsoft.WindowsDesktop.Internal.Transport ?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 13, 2021

Name of the transport package is little confusing to me. Should it be sounded like Microsoft.RuntimeForWindowsDesktop.Internal.Transport instead of Microsoft.WindowsDesktop.Internal.Transport ?

We already deliver a transport package to aspnetcore with same naming schema: Microsoft.AspNetCore.Internal.Transport. If possible I would like to keep them consistent. It's a non shipping package anyway (it never appears on nuget.org).

@dreddy-work
Copy link
Member

Name of the transport package is little confusing to me. Should it be sounded like Microsoft.RuntimeForWindowsDesktop.Internal.Transport instead of Microsoft.WindowsDesktop.Internal.Transport ?

We already deliver a transport package to aspnetcore with same naming schema: Microsoft.AspNetCore.Internal.Transport. If possible I would like to keep them consistent. It's a non shipping package anyway (it never appears on nuget.org).

We have winforms transport package with name Microsoft.Winforms.* that contains bits from winforms to Desktop repo . With this notion, name is telling me that it is a windowsdesktop transport package but in reality, its a runtime supporting/dependancy package for desktop?

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 13, 2021

We have winforms transport package with name Microsoft.Winforms.* that contains bits from winforms to Desktop repo . With this notion, name is telling me that it is a windowsdesktop transport package but in reality, its a runtime supporting/dependancy package for desktop?

In principle I agree with you but how important is naming here given that it's an internal package only and will never appear on any public feed? Also we would be trading consistency in dotnet/runtime in favor of consistency in dotnet/windowsdesktop.

@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

I agree with @dreddy-work on the name concern - we have a number of the same packages in dotnet/winforms, and I'd think we'd need to replace those with the same package. https:/dotnet/winforms/blob/5f5b89a28c8905b47d46a27ea78ed293fca25ac6/eng/Versions.props#L13-L40

Now it'd look rather strange if dotnet/winforms referenced Microsoft.WindowsDesktop.Internal.Transport, but it'd look less strange and meaningful if it referenced Microsoft.RuntimeForWindowsDesktop.Internal.Transport.
I'd argue that the chosen naming convention lacks "origin", and it'd be significantly better if it was something like Microsoft.[Internal|Private].Runtime.[AspNetCore|WindowsDesktop|etc.]. In fact it'd be even better if we aligned names of all dotnet transport packages.

@ViktorHofer
Copy link
Member Author

ViktorHofer commented Aug 16, 2021

I agree with @dreddy-work on the name concern - we have a number of the same packages in dotnet/winforms, and I'd think we'd need to replace those with the same package. https:/dotnet/winforms/blob/5f5b89a28c8905b47d46a27ea78ed293fca25ac6/eng/Versions.props#L13-L40

It's important that dotnet/windowsdesktop which composes the shared framework needs to use this transport package as otherwise implementation assemblies would be redistributed into the windowsdesktop targeting pack. For that reason, the transport package only contains the assemblies that are built in dotnet/runtime and are part of the targeting pack.

dotnet/winforms and dotnet/wpf could change to use the transport package for the overlapping set of assemblies but the transport's target is the windowsdesktop shared framework. It's purpose isn't to bundle together any assembly that windowsdesktop, winforms or wpf might need from dotnet/runtime.

In fact it'd be even better if we aligned names of all dotnet transport packages.

Apart from consistency and a decent amount of churn across the stack, it's unclear to me what the benefit of that would be for the customer? Not saying that we shouldn't do it but it requires a business justification. In addition to that, without establishing a policy and protection (i.e. on the publishing side of Arcade), such consistency will likely regress again in the future.

[Internal|Private].Runtime.[AspNetCore|WindowsDesktop|etc.].

@dougbu @wtgodbe do you share the same concern for aspnetcore? I would like to keep the transport package names in dotent/runtime consistent. So ideally if we change this one I would want to change the aspnetcore's transport package name.

Now it'd look rather strange if dotnet/winforms referenced Microsoft.WindowsDesktop.Internal.Transport, but it'd look less strange and meaningful if it referenced Microsoft.RuntimeForWindowsDesktop.Internal.Transport.
I'd argue that the chosen naming convention lacks "origin", and it'd be significantly better if it was something like Microsoft.[Internal|Private].Runtime.[AspNetCore|WindowsDesktop|etc.].

Works for me. My only concern is that we need to get this into windowsdesktop before we snap (tomorrow). As I'm out tomorrow I would like to agree on a name asap.

@dreddy-work @RussKie should we go with Microsoft.RuntimeForWindowsDesktop.Internal.Transport or Microsoft.Internal.Runtime.WindowsDesktop.Transport?

@RussKie
Copy link
Member

RussKie commented Aug 16, 2021

I'd go with Microsoft.Internal.Runtime.WindowsDesktop.Transport, and later we could add Microsoft.Internal.Runtime.WindowsForms.Transport and Microsoft.Internal.Runtime.Wpf.Transport if necessary

@wtgodbe
Copy link
Member

wtgodbe commented Aug 16, 2021

I'm a fan of having the transport packages follow a consistent naming scheme. Microsoft.Internal.Runtime.Aspnetcore.Transport works for me.

ViktorHofer added a commit to ViktorHofer/runtime that referenced this pull request Aug 16, 2021
As agreed on in dotnet/windowsdesktop#1936, we
want to follow a common schema when naming our transport packages.

Also updating the docs to reflect the name changes and to also
accomodate for the WindowsDesktop transport package.
@ViktorHofer
Copy link
Member Author

Awesome. Submitted dotnet/runtime#57504 to fix this.

@wtgodbe you will need to change the package name in the next update dependencies PR in aspnetcore that updates the dotnet/runtime bits. (More precisely the Versions.props property rename, Version.Details.xml dependency update and the PackageReference Include="" and Version="").

@dreddy-work
Copy link
Member

@ViktorHofer , can you merge or rebase to resolve conflicts?

@ViktorHofer
Copy link
Member Author

@ViktorHofer , can you merge or rebase to resolve conflicts?

Waiting to get the new package in first.

ViktorHofer added a commit to dotnet/runtime that referenced this pull request Aug 16, 2021
* Rename transport packages to follow convention

As agreed on in dotnet/windowsdesktop#1936, we
want to follow a common schema when naming our transport packages.

Also updating the docs to reflect the name changes and to also
accomodate for the WindowsDesktop transport package.

* Update src.proj

* Update src.proj
@ViktorHofer
Copy link
Member Author

The PR is now ready. Would appreciate a review.

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.

4 participants