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

[main] Add Microsoft.Internal.Runtime.WindowsDesktop.Transport dependency for CPD #5510

Merged
merged 1 commit into from
Aug 25, 2021

Conversation

mmitche
Copy link
Member

@mmitche mmitche commented Aug 23, 2021

null

Microsoft Reviewers: Open in CodeFlow

@mmitche mmitche requested a review from a team as a code owner August 23, 2021 23:13
@ghost ghost assigned mmitche Aug 23, 2021
@mmitche mmitche changed the title Add Microsoft.Internal.Runtime.WindowsDesktop.Transport dependency for CPD [main] Add Microsoft.Internal.Runtime.WindowsDesktop.Transport dependency for CPD Aug 23, 2021
@RussKie
Copy link
Member

RussKie commented Aug 23, 2021

@ViktorHofer now we have the transport package as well as loose packages that are present in the transport, which feels like we're unnecessarily duplicating. Circling back to my original question - should we remove the loose package references?

@dreddy-work

This comment has been minimized.

@dreddy-work

This comment has been minimized.

@mmitche
Copy link
Member Author

mmitche commented Aug 23, 2021

You could remove the loose ones if they are duplicates. This change is for windowsdesktop, so that it can take a CPD here: https://github.com/dotnet/windowsdesktop/pull/1994/files

@dreddy-work
Copy link
Member

Looks like i do not have permission to push changes. @mmitche / @RussKie , can you try pushing the changes by removing duplicates similar to what is done here: dotnet/windowsdesktop#1936

@ViktorHofer
Copy link
Member

@ViktorHofer now we have the transport package as well as loose packages that are present in the transport, which feels like we're unnecessarily duplicating. Circling back to my original question - should we remove the loose package references?

I still don't understand why you want to do this. The transport package contains many reference and implementation assemblies. When referencing the package inside a project, all assemblies are referenced, even ones which aren't part of the project's dependency graph.

For repositories which compose their own shared frameworks, I prefer to have fine grained dependencies instead of one package that references everything. That said, if your individual projects in winforms/wpf already reference all assemblies that come via the transport package, then yes, you should remove the individual PackageReferences.

@mmitche
Copy link
Member Author

mmitche commented Aug 24, 2021

Looks like i do not have permission to push changes. @mmitche / @RussKie , can you try pushing the changes by removing duplicates similar to what is done here: dotnet/windowsdesktop#1936

@dreddy-work Did you try to push to this branch in my fork? If you have write access to winforms you should have write access to this branch.

@dreddy-work
Copy link
Member

dreddy-work commented Aug 24, 2021

Looks like i do not have permission to push changes. @mmitche / @RussKie , can you try pushing the changes by removing duplicates similar to what is done here: dotnet/windowsdesktop#1936

@dreddy-work Did you try to push to this branch in my fork? If you have write access to winforms you should have write access to this branch.

I tried. Got some authentication errors. May be we can take that in the next PR.

@dreddy-work
Copy link
Member

@ViktorHofer now we have the transport package as well as loose packages that are present in the transport, which feels like we're unnecessarily duplicating. Circling back to my original question - should we remove the loose package references?

I still don't understand why you want to do this. The transport package contains many reference and implementation assemblies. When referencing the package inside a project, all assemblies are referenced, even ones which aren't part of the project's dependency graph.

For repositories which compose their own shared frameworks, I prefer to have fine grained dependencies instead of one package that references everything. That said, if your individual projects in winforms/wpf already reference all assemblies that come via the transport package, then yes, you should remove the individual PackageReferences.

Is it possible to go out of sync with respect to version numbers on direct references in project vs references packaged in the Transport package?.

@mmitche
Copy link
Member Author

mmitche commented Aug 24, 2021

@ViktorHofer now we have the transport package as well as loose packages that are present in the transport, which feels like we're unnecessarily duplicating. Circling back to my original question - should we remove the loose package references?

I still don't understand why you want to do this. The transport package contains many reference and implementation assemblies. When referencing the package inside a project, all assemblies are referenced, even ones which aren't part of the project's dependency graph.
For repositories which compose their own shared frameworks, I prefer to have fine grained dependencies instead of one package that references everything. That said, if your individual projects in winforms/wpf already reference all assemblies that come via the transport package, then yes, you should remove the individual PackageReferences.

Is it possible to go out of sync with respect to version numbers on direct references in project vs references packaged in the Transport package?.

Possibly around incremental servicing. Need to answering the following questions:

  • Are all contents within the transport always going to be shipped as separate packages to nuget.org on each release (or never shipped to nuget at all)
  • If one of the binaries in the transport is only shipped every once in a while in a separate package, will it only appear in the transport if it's also being packaged?

@ViktorHofer, that's a relevant question with this PR too: https://github.com/dotnet/windowsdesktop/pull/1936/files. For instance, Microsoft.Win32.SystemEvents appears to be serviced incrementally for 5.0. If it is always in the transport package, we'll end up with a different version shipped to nuget.org than in the WD shared framework.

@ViktorHofer
Copy link
Member

@ViktorHofer, that's a relevant question with this PR too: https://github.com/dotnet/windowsdesktop/pull/1936/files. For instance, Microsoft.Win32.SystemEvents appears to be serviced incrementally for 5.0. If it is always in the transport package, we'll end up with a different version shipped to nuget.org than in the WD shared framework.

If a library is updated and ships independently (either public to nuget.org or internal) but is also part of a transport package, then the transport package needs to ship during incremental servicing as well. That's the process that we want to follow for NET6 servicing.

Are all contents within the transport always going to be shipped as separate packages to nuget.org on each release (or never shipped to nuget at all)

The transport packages that dotnet/runtime produce never ship publicly.

If one of the binaries in the transport is only shipped every once in a while in a separate package, will it only appear in the transport if it's also being packaged?

If a library that is part of a transport package is updated, the transport package needs to published as well.

@mmitche
Copy link
Member Author

mmitche commented Aug 24, 2021

If a library that is part of a transport package is updated, the transport package needs to published as well.

What I'm getting at is let's say it's Microsoft.Win32.Registry.dll. At 6.0.0 that will appear in both the Microsoft.Win32.Registry nuget package that ships to nuget.org and the transport package, as expected. What happens for 6.0.1 though, if there are no fixes to Microsoft.Win32.Registry.dll? It sounds like it would still have to be within the transport package because that's how windows desktop is consuming Microsoft.Win32.Registry.dll, right (I assume the build would fail otherwise)? But in order for that to happen, you'd have to rebuild that dll in the 6.0.1 runtime build using the 6.0.1 sources. That would lead to a different file version appearing in the windows desktop shared framework for 6.0.1 vs. what is the latest on nuget.org, which only shipped a version at 6.0.0.

@ViktorHofer
Copy link
Member

What happens for 6.0.1 though, if there are no fixes to Microsoft.Win32.Registry.dll? It sounds like it would still have to be within the transport package because that's how windows desktop is consuming Microsoft.Win32.Registry.dll, right (I assume the build would fail otherwise)?

During incremental servicing a package for Microsoft.Internal.Runtime.WindowsDesktop.Transport is only created if any of its content has changed. Neither package versions nor assembly versions increment automatically if nothing has changed in a library.

If nothing changed, then the transport package won't publish during 6.0.1 and windowsdesktop will continue to consume the 6.0.0 one.

Please let me know if I'm missing something here.

@dreddy-work
Copy link
Member

What happens for 6.0.1 though, if there are no fixes to Microsoft.Win32.Registry.dll? It sounds like it would still have to be within the transport package because that's how windows desktop is consuming Microsoft.Win32.Registry.dll, right (I assume the build would fail otherwise)?

During incremental servicing a package for Microsoft.Internal.Runtime.WindowsDesktop.Transport is only created if any of its content has changed. Neither package versions nor assembly versions increment automatically if nothing has changed in a library.

If nothing changed, then the transport package won't publish during 6.0.1 and windowsdesktop will continue to consume the 6.0.0 one.

Please let me know if I'm missing something here.

Two things here:

  • We are confirming the versions won't go out of sync between transport package and the nuget org.
  • Arcade/Runtime dependency flow will make sure both transport package version and loose reference versions in this repo are updated together when needed.

If this is case, I have no further questions and happy to merge.

@ViktorHofer
Copy link
Member

@mmitche and I synced up earlier today and discussed this and he is fine with the expected flow during (incremental) servicing.

We are confirming the versions won't go out of sync between transport package and the nuget org.
Arcade/Runtime dependency flow will make sure both transport package version and loose reference versions in this repo are updated together when needed.

Correct, versions between the transport package and the other library shipping packages (on nuget.org / dotnet6 feed) won't get out of sync. Agreed as well, dependency flow will make sure that those are in sync.

@dreddy-work dreddy-work merged commit afcb6fe into dotnet:main Aug 25, 2021
@ghost ghost added this to the 7.0 alpha1 milestone Aug 25, 2021
@ghost ghost locked as resolved and limited conversation to collaborators Jan 27, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants