Skip to content

[release/3.1] Expose all references when not restoring #33197

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 6 commits into from
Sep 8, 2021
Merged

[release/3.1] Expose all references when not restoring #33197

merged 6 commits into from
Sep 8, 2021

Conversation

@dougbu
Copy link
Contributor

@dougbu dougbu commented Jun 1, 2021

  • Expose all references when not restoring
    • use empty $(MSBuildRestoreSessionId) to determine when contributing to dependency graph
  • Remove extra direct references
    • should now be part of the dependency graph automatically
  • Avoid errors about non-shared Fx references
    • not a problem unless executing restore target
  • Special case source builds

@dougbu dougbu added area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode labels Jun 1, 2021
@dougbu dougbu added this to the 3.1.17 milestone Jun 1, 2021
@dougbu dougbu requested a review from a team June 1, 2021 20:14
@wtgodbe
Copy link
Member

wtgodbe commented Jun 1, 2021

Looks like this is causing SharedFx tests to fail

SharedFrameworkContainsListedAssemblies

Assert.Empty() Failure\nCollection: ["System.Text.Json", "Microsoft.Bcl.AsyncInterfaces"]

at Microsoft.AspNetCore.SharedFxTests.SharedFrameworkContainsListedAssemblies() in /_/src/Framework/test/SharedFxTests.cs:line 50

@Tratcher Tratcher removed their request for review June 18, 2021 21:06
@dougbu
Copy link
Contributor Author

dougbu commented Jun 19, 2021 •
edited
Loading

This is blocked for now because the Arcade SDK doesn't set $(MSBuildRestoreSessionId) when restoring the repo. I tried back-porting dotnet/arcade@c2875f0932d but that caused new issues related (I think) to broader use of Helix.targets in the release/3.x branch. I tried back-porting dotnet/arcade@961b7d2e1c7 as well to resolve those new problems but, while it minimized the differences git diff release/5.0 -- src/Microsoft.DotNet.Arcade.Sdk/tools/Build.proj shows (mostly to publish bits), the result was lots of new warnings in my local builds of dotnet/aspnetcore.

/btw dotnet/arcade@c2875f0932d alone doesn't work in release/3.x because that uses a <PropertyGroup> to create @(_SolutionRestoreProps) items. I needed to move that into an <ItemGroup>, perhaps due to the different .NET SDK. (release/5.0 builds are fine and we use desktop msbuild at least some of the time in both branches.) Any suggestions @rainersigwald @ViktorHofer❔

@dougbu dougbu mentioned this pull request Jul 13, 2021
Merged
1 task
@dougbu dougbu force-pushed the dougbu/more.conditional.conditions/3.1 branch from d15c7a6 to 2f06b0d Compare July 14, 2021 05:21
@dougbu dougbu force-pushed the dougbu/more.conditional.conditions/3.1 branch 2 times, most recently from 75915c0 to 2f26e3a Compare July 30, 2021 23:38
@dougbu
Copy link
Contributor Author

dougbu commented Jul 31, 2021

The current changes do not remove all CS1705 workarounds. Still need a lot of <CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies> in our projects to avoid compiling against our ref/ assemblies. These workarounds are specific to this branch and help e.g. with transitive references through EF assemblies.

@dougbu
Copy link
Contributor Author

dougbu commented Jul 31, 2021

Still need a lot of <CompileUsingReferenceAssemblies>false</CompileUsingReferenceAssemblies> in our projects

Good news is these workarounds are all in test (or test asset), sample, and (in one case) benchmark projects.

@dougbu dougbu added the blocked The work on this issue is blocked due to some dependency label Jul 31, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Jul 31, 2021

Temporarily blocked on dotnet/arcade#7612 and an Arcade dependency update in our 'release/3.1' after that. I'll rebase and remove the extra commits to use 'General Testing' assemblies when that'll work…

@dougbu
Copy link
Contributor Author

dougbu commented Jul 31, 2021

Please review but ignore 92376e7 changes

wtgodbe
wtgodbe approved these changes Aug 2, 2021
View reviewed changes
Copy link
Member

@wtgodbe wtgodbe left a comment

Choose a reason for hiding this comment

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

LGTM

dougbu added 5 commits August 19, 2021 15:47
@dougbu
[release/3.1] Expose all references when not restoring
f85b424 
- backport of 0b0bed3 (#32718)

* Expose all references when not restoring
  - use empty `$(MSBuildRestoreSessionId)` to determine when contributing to dependency graph
* Remove extra direct references
  - should now be part of the dependency graph automatically
* Avoid errors about non-shared Fx references
  - not a problem unless executing `restore` target
* Special case source builds
@dougbu
Add workaround for ref/ generation
51e347e 
- use `$(CompileUsingReferenceAssemblies)` to avoid new references in generated projects
@dougbu
Limit _CheckForReferenceBoundaries to Core TFM
650b74b 
- back-ports a very small part of 5266918
- avoid incorrect errors about extra references in shared framework
- safe because every `$(IsAspNetCoreApp)` project targets (or multi-targets) `netcoreapp3.1`
@dougbu
Add more $(MSBuildRestoreSessionId) checks
6a7303b 
- additional projects in this branch have TFM-specific references
@dougbu
nit: Remove a few duplicate references
d0af3f2 
- no need to mention Microsoft.AspNetCore.DataProtection twice
@dougbu dougbu force-pushed the dougbu/more.conditional.conditions/3.1 branch from 1a79d3b to d0af3f2 Compare August 19, 2021 22:49
@dougbu
Remove excess assemblies from shared Fx
5dbc193 
- brought in due to improved reference discovery
@dougbu dougbu removed the blocked The work on this issue is blocked due to some dependency label Aug 22, 2021
@dougbu
Copy link
Contributor Author

dougbu commented Aug 22, 2021

This is no longer blocked and is ready for 3.1.20.

@dougbu dougbu modified the milestones: 3.1.17, 3.1.20 Aug 22, 2021
@wtgodbe
Copy link
Member

wtgodbe commented Aug 24, 2021

My memory is foggy now, what problem was this trying to solve?

@dougbu
Copy link
Contributor Author

dougbu commented Aug 24, 2021

what problem was this trying to solve?

Remember our STEW and STJ issues in 5.0.x❔ That problem stemmed from not finding transitive references and thereby missing unexpected assembly versions in our tests. This change ensures all assemblies from NETCore.App and the transport package (in later branches) are exposed, ship correctly, and verified in our tests.

@dougbu dougbu merged commit ae2b241 into release/3.1 Sep 8, 2021
@dougbu dougbu deleted the dougbu/more.conditional.conditions/3.1 branch September 8, 2021 23:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@wtgodbe wtgodbe wtgodbe approved these changes

@BrennanConroy BrennanConroy Awaiting requested review from BrennanConroy BrennanConroy is a code owner

@halter73 halter73 Awaiting requested review from halter73 halter73 is a code owner

@jkotalik jkotalik Awaiting requested review from jkotalik

@SteveSandersonMS SteveSandersonMS Awaiting requested review from SteveSandersonMS

Assignees

No one assigned

Labels

area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework tell-mode Indicates a PR which is being merged during tell-mode

Projects

None yet

Milestone

3.1.20

Development

Successfully merging this pull request may close these issues.

3 participants

@dougbu @wtgodbe