Skip to content

Simplify ref/ assembly generation #24136

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 2 commits into from
Jul 21, 2020
Merged

Simplify ref/ assembly generation #24136

merged 2 commits into from
Jul 21, 2020

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Jul 20, 2020

  • followup 1/2 for 5266918
  • correct the Razor.Tools project
    • %(Reference.Version) metadata does not bleed through into @(PackageReference) items
    • much more work to do so than to add this special case
  • remove RTMVersions project and use RepoTasks instead
    • make it an error if RepoTasks is not restored before anything else builds
  • add items and properties for System.Security.AccessControl

nits:

  • remove invalid (ignored) metadata in Directory.Build.props and AzureAppServices.SiteExtension project
  • improve / extend a couple of comments

@dougbu dougbu requested review from pranavkm, NTaylorMullen and a team July 20, 2020 23:06
pranavkm
pranavkm approved these changes Jul 20, 2020
View reviewed changes
Copy link
Contributor

@pranavkm pranavkm left a comment

Choose a reason for hiding this comment

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

Razor TOols changes look fine

dougbu
dougbu commented Jul 21, 2020
View reviewed changes
eng/Versions.props

If testing this configuration prior to servicing, update the versions of dependencies too. E.g. change
`$(SystemSecurityPrincipalWindowsV0PackageVersion)` if you change `$(SystemSecurityAccessControlV0PackageVersion)`
because System.Security.AccessControl will otherwise be loadable. This should not be necessary in servicing.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@ericstj when I updated only $(SystemSecurityAccessControlV0PackageVersion) in testing, the newer (non-RTM) copy of System.Security.AccessControl.dll ended up in our targeting pack. I believe the newer assembly was also used in most compilations. It was difficult to tell exactly why but my guess is the newer System.Security.Principal.Windows.dll was somehow considered incompatible and conflict resolution chose the newer version of both. (The System.Security.Principal.Windows package is a direct dependency of a few of our packages.)

Will we hit something similar if dotnet/runtime ships System.Security.AccessControl 5.0.1 but leaves System.Security.Principal.Windows at 5.0.0❔ Put another way, will dotnet/runtime build but not ship a newer System.Security.Principal.Windows.dll and use that when compiling System.Security.AccessControl.dll❔ If yes, we'll need to be really careful here (perhaps adding further workarounds) and I shouldn't include the "not be necessary" words.

Copy link
Member

Choose a reason for hiding this comment

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

There’s nothing fancy here. It just chooses the newer version. You’ll only see things go app local if you use a newer package than what’s in the shared framework you target.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't generally use our own part of the shared Fx when building and the example assemblies are in Microsoft.AspNetcore.App.Ref, not Microsoft.NETCore.App.Ref. So, does "nothing fancy" help us with the corner case I describe❔

I'd like to know we can rev one without the other reliably before we get to 5.0.1. (And, these packages / assemblies have a few other inter-dependencies.)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unless someone objects, I'm going to assume the worst case here would be silently compiling against 5.0.x (newer) versions of these packages when servicing. Will file an issue about testing that %(RTMVersion) metadata is respected. That test will likely be a bit painful since we won't be building our targeting pack when servicing and checks will require looking at all references in our implementation assemblies for the relevant assemblies then checking their [AssemblyInformationalVersion()] attributes against the expected package version.

@dougbu
Copy link
Contributor Author

dougbu commented Jul 21, 2020

Side question: I see ExceptionHandlerTest.ClearsResponseBuffer_BeforeRequestIsReexecuted(...) failing in https://dev.azure.com/dnceng/public/_build/results?buildId=737724 (a build of master this morning) and https://dev.azure.com/dnceng/public/_build/results?buildId=738552 (the validation build for this PR). @JunTaoLuo is this a newly-flaky test you're about to quarantine❔

@dougbu
Copy link
Contributor Author

dougbu commented Jul 21, 2020

/azp run

@azure-pipelines
Copy link

Azure Pipelines successfully started running 2 pipeline(s).

@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jul 21, 2020
wtgodbe
wtgodbe reviewed Jul 21, 2020
View reviewed changes
eng/targets/ResolveReferences.targets
@@ -205,21 +205,23 @@
</Target>

<!--
Muck with @(ResolvedCompileFileDefinitions) items between generation and use in order to compile against RTM lib/
Change @(ResolvedCompileFileDefinitions) items between generation and use in order to compile against RTM lib/
Copy link
Member

Choose a reason for hiding this comment

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

Good change 😆

wtgodbe
wtgodbe reviewed Jul 21, 2020
View reviewed changes
dougbu added 2 commits July 21, 2020 12:19
@dougbu
Simplify ref/ assembly generation
94aa417 
- followup 1/2 for 5266918
- correct the Razor.Tools project
  - `%(Reference.Version)` metadata does not bleed through into `@(PackageReference)` items
  - much more work to do so than to add this special case
- remove RTMVersions project and use RepoTasks instead
  - make it an error if RepoTasks is not restored before anything else builds
- add items and properties for System.Security.AccessControl

nits:
- remove invalid (ignored) metadata in Directory.Build.props and AzureAppServices.SiteExtension project
- improve / extend a couple of comments
@dougbu
!fixup! Instead remove $(Razor_NewtonsoftJsonPackageVersion)
a8f41b2 
nits:
- remove redundant `%(Reference.Version)` metadata for Microsoft.Css.Parser too
- move `@(Reference)` items together in Microsoft.AspNetCore.Razor.Tools
@dougbu dougbu force-pushed the dougbu/simplify.RTMVersions.5266918ed2be branch from 6cef541 to a8f41b2 Compare July 21, 2020 19:20
@dougbu dougbu merged commit fb28ce3 into master Jul 21, 2020
@dougbu dougbu deleted the dougbu/simplify.RTMVersions.5266918ed2be branch July 21, 2020 22:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@JunTaoLuo JunTaoLuo JunTaoLuo left review comments

@ericstj ericstj ericstj left review comments

@wtgodbe wtgodbe wtgodbe approved these changes

@pranavkm pranavkm pranavkm approved these changes

@NTaylorMullen NTaylorMullen Awaiting requested review from NTaylorMullen

Assignees
No one assigned
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@dougbu @pranavkm @NTaylorMullen @JunTaoLuo @ericstj @wtgodbe @Pilchie