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

Major per-RuntimeIdentifier build refactoring #218

Merged
merged 1 commit into from Apr 3, 2020

Conversation

andrew-boyarshin
Copy link
Contributor

@andrew-boyarshin andrew-boyarshin commented Mar 28, 2020

  • ExtrasIncludeDefaultProjectBuildOutputInPackTarget allows overriding the target which packs the RID-specific build output (assemblies and symbols), fixes Per-RuntimeIdentifier packaging doesn't allow overriding build output path in nupkg #217.
  • Update to latest Microsoft.Common.CrossTargeting.targets upstream source code.
  • _SdkPrepareProjectFlavorMatrix is now responsible for computing project flavors matrix: TargetFramework × RuntimeIdentifier.
  • Reimplement _ComputeTargetFrameworkItems and _WalkEachTargetPerFramework on top of _SdkPrepareProjectFlavorMatrix
  • Mimic ProjectReference item spec to simplify _WalkEachTargetPerFramework (by removing duplication)
  • Use MSBuildProjectFullPath consistently, avoid MSBuildProjectFile or mixing those two.
  • Drop no-op _ExtrasPackageRuntimeFiles.
  • Pay attention to unnecessary metadata, use KeepMetadata where applicable (opt-out possible and verified).

PackagePath override sample

@andrew-boyarshin
Copy link
Contributor Author

@clairernovotny it would be great to have this PR reviewed.

@clairernovotny
Copy link
Collaborator

In general looks good. Can you please add a sample/test project that shows the overriding?

Also, what's is the benefit of removing the extra metadata vs the extra complexity that's added? What do we get for it?

@andrew-boyarshin
Copy link
Contributor Author

@clairernovotny sure, I'll make a test project and update README to mention the ability to override paths.

My rationale: This project is the foundation of many .NET projects. It should be written like a part of .NET/MSBuild SDK, with practices used there. In particular, the per-RID build feature is so similar to ProjectReferences. ProjectReferences remove extra metadata to reduce GC footprint. Therefore, MSBuildSdkExtras should do the same.

@andrew-boyarshin
Copy link
Contributor Author

andrew-boyarshin commented Apr 3, 2020

@clairernovotny done. I've removed KeepDuplicates since the outputs should be unique anyway, no need to add extra Distinct call.

P.S. merge with squash recommended.

upd: NOMERGE

@clairernovotny
Copy link
Collaborator

@andrew-boyarshin just lmk when things are ready.

@andrew-boyarshin
Copy link
Contributor Author

@clairernovotny fixed and expanded test set (they were using TargetFramework instead of TargetFrameworks), refactored targets to mimic the original more closely (different source of TargetFramework matrix row for different targets), added verification of RID-specific build outputs with MSBuild warning and tests.

Finally ready to be reviewed and merged.

@clairernovotny clairernovotny merged commit b0fa124 into novotnyllc:master Apr 3, 2020
@clairernovotny
Copy link
Collaborator

Thanks!

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.

Per-RuntimeIdentifier packaging doesn't allow overriding build output path in nupkg
2 participants