Skip to content
This repository was archived by the owner on Jan 23, 2023. It is now read-only.

Make CoreFx.Private.TestUtilites RID-agnostic#36207

Merged
ViktorHofer merged 6 commits intodotnet:masterfrom
ViktorHofer:Win32RegistryHarvest
Jun 29, 2019
Merged

Make CoreFx.Private.TestUtilites RID-agnostic#36207
ViktorHofer merged 6 commits intodotnet:masterfrom
ViktorHofer:Win32RegistryHarvest

Conversation

@ViktorHofer
Copy link
Member

@ViktorHofer ViktorHofer commented Mar 21, 2019

Changes:

  • Downgrade CoreFx.Private.TestUtilities to a "normal" test project and remove its packaging

EDIT: Removed the harvesting part from this PR

@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch from a50f1f0 to ef410f7 Compare March 21, 2019 15:45
@ViktorHofer
Copy link
Member Author

ViktorHofer commented Mar 21, 2019

@ericstj is there a way to suppress the mismatch error here? Microsoft.Win32.SystemEvents has mismatched compile (4.0.1.0) and runtime (4.0.0.0) versions on .NETCoreApp,Version=v2.0/win7-x86..

I understand the issue but as we live build the ref which is used for the harvested asset (netcoreapp2.0) that assembly version will always be higher.

image

@ViktorHofer
Copy link
Member Author

@ericstj I added a netcoreapp2.0 ref live build asset because of an AssemblyVersion mismatch between the lib and ref.

@ViktorHofer
Copy link
Member Author

@ericstj ok this didn't work well. I now pinned the assembly version to 4.0.0.0. Let me know if there's a better approach.

@ericstj
Copy link
Member

ericstj commented Mar 21, 2019

Let me know if there's a better approach.

Per my above comment don't harvest the lib.

When thinking about these packages you should try to imagine the "meaningful" differences in the libraries. I'd assert that netcoreapp2.0 vs netcoreapp (latest) isn't a meaningful distinction since the library isn't adding new API nor is it trying to make use of new API we've added to the framework. As such you shouldn't harvest the netcoreapp2.0 asset as it is the latest meaningfully significant configuration for the library. Also consider that multiple configurations without meaningful differences should be avoided as it bloats the package.

If you happen to harvest a lib you need to harvest all refs that might apply to the same framework, so if you harvest netcoreapp2.0 you need to make sure you harvest the netcoreapp2.0 ref and netstandard2.0 ref since neither one is allowed to change as it would make the assembly version greater than the harvested one. This could force you to add a desktop configuration for a ref in some case if you are still live building the desktop impl. That said, I don't think you should do any of that here.

@ViktorHofer
Copy link
Member Author

Thanks, that makes perfectly sense. I stopped the netcoreapp2.0 harvesting and will review the pkgprojs when I'm back from holiday.

@ViktorHofer ViktorHofer changed the title Harvest Registry packages for netfx and netcoreapp2.0 Harvest Registry packages for netfx Mar 23, 2019
@karelz karelz added this to the 3.0 milestone Apr 1, 2019
@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch from 21e143b to 1fda7d4 Compare April 3, 2019 14:05
@ViktorHofer
Copy link
Member Author

As Microsoft.Win32.Registry is still needed in multiple projects that target netfx I followed the same approach that I took for System.Memory and binplaced the harvested assets. I added ProjectExclusions to not build the project anymore (as it still has a netstandard configuration which would be chosen). Unfortunately it seems that the exclusions break the P2P reference from System.IO.Ports to Microsoft.Win32.Registry when System.IO.Ports targets netstandard instead of netfx (see offline mail thread why it does so during a netfx run).

@ericstj thoughts?

@ericstj
Copy link
Member

ericstj commented Apr 3, 2019

break the P2P reference from System.IO.Ports to Microsoft.Win32.Registry

There is no P2P reference. There is a simple name reference and you stopped producing the 'netstandard' ref for Registry. You either need to ensure you still build that or binplace it. Note that binplacing it may be a problem for source build. https://github.com/dotnet/corefx/issues/36451

@ViktorHofer
Copy link
Member Author

Sorry for mixing up terms, you are right this is just a simple name reference.

I cherry-picked the configuration fix for System.IO.Pipes ef527a1 into this PR and the error is now gone as in the netfx build the netstandard version of System.IO.Pipes isn't build anymore and therefore it doesn't require the netstandard version of Microsoft.Win32.Registry during the netfx vertical build anymore.

@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch from 7dc28ae to 56df45c Compare April 3, 2019 22:32
@ViktorHofer ViktorHofer requested a review from ericstj April 3, 2019 22:36
@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch 2 times, most recently from 1ee6eb3 to 17d2a6d Compare April 9, 2019 21:36
@ViktorHofer ViktorHofer changed the title Harvest Registry packages for netfx Downgrade CoreFx.Private.TestUtilites and harvest Microsoft.Win32.Registry* and System.Diagnostics.Process Apr 9, 2019
@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch 2 times, most recently from 5186004 to cda7fae Compare April 10, 2019 14:09
@ViktorHofer
Copy link
Member Author

@ericstj I factored out the harvesting changes and this now only deals with CoreFx.Private.TestUtilities. Can you please take a look?

@ViktorHofer ViktorHofer changed the title Downgrade CoreFx.Private.TestUtilites Make CoreFx.Private.TestUtilites RID-agnostic Jun 28, 2019
Copy link
Member

@ericstj ericstj left a comment

Choose a reason for hiding this comment

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

Looks reasonable. Thanks for pulling out registry changes.

@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch 3 times, most recently from d50da49 to 8c595ab Compare June 29, 2019 11:32
@ViktorHofer
Copy link
Member Author

OK, VS experience now works with the changes :)

@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch from 8c595ab to 3f18465 Compare June 29, 2019 16:51
@ViktorHofer ViktorHofer changed the title Make CoreFx.Private.TestUtilites RID-agnostic Make CoreFx.Private.TestUtilites RID-agnostic and generate framework manifest Jun 29, 2019
@ViktorHofer ViktorHofer force-pushed the Win32RegistryHarvest branch from bebd61a to c486166 Compare June 29, 2019 18:54
@ViktorHofer ViktorHofer changed the title Make CoreFx.Private.TestUtilites RID-agnostic and generate framework manifest Make CoreFx.Private.TestUtilites RID-agnostic Jun 29, 2019
@ViktorHofer ViktorHofer merged commit d3d144f into dotnet:master Jun 29, 2019
@ViktorHofer ViktorHofer deleted the Win32RegistryHarvest branch June 29, 2019 19:53
@karelz karelz modified the milestones: Future, 3.0 Jul 16, 2019
picenka21 pushed a commit to picenka21/runtime that referenced this pull request Feb 18, 2022
* Make CoreFx.Private.TestUtilites RID-agnostic

* Add version checks for other distros

https://github.com/dotnet/corefx/commit/dotnet/corefx@4f0d10773ccbb4849b869b6a0a73d5938f1804bd
Porting fix over into PlatformDetection.

* Build test lib in advance for VS support

* Disable pretest in AllConfigurations

Commit migrated from dotnet/corefx@d3d144f
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