Skip to content

Always build App.Ref and the targeting packs #39568

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

Merged
merged 1 commit into from
Jan 19, 2022

Conversation

dougbu
Copy link
Contributor

@dougbu dougbu commented Jan 17, 2022

  • remove $(IsTargetingPackBuilding) entirely

- remove `$(IsTargetingPackBuilding)` entirely
@dougbu dougbu force-pushed the dougbu/istargetingpackbuilding.yes.39471 branch from 7d3e480 to 0bcb79d Compare January 17, 2022 07:18
dougbu added a commit to dougbu/aspnetcore that referenced this pull request Jan 17, 2022
- subset of dotnet#39568
- set `$(IsTargetingPackBuilding)` to `true` unconditionally
- leave all _use_ of `$(IsTargetingPackBuilding)`
@Pilchie Pilchie added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 18, 2022
@dougbu
Copy link
Contributor Author

dougbu commented Jan 18, 2022

/backport to release/6.0

@github-actions
Copy link
Contributor

Started backporting to release/6.0: https://github.com/dotnet/aspnetcore/actions/runs/1715068469

@github-actions
Copy link
Contributor

@dougbu backporting to release/6.0 failed, the patch most likely resulted in conflicts:

$ git am --3way --ignore-whitespace --keep-non-patch changes.patch

Applying: Always build App.Ref and the targeting packs - remove `$(IsTargetingPackBuilding)` entirely
Using index info to reconstruct a base tree...
M	Directory.Build.props
M	Directory.Build.targets
M	eng/Versions.props
M	eng/targets/ResolveReferences.targets
M	src/Framework/App.Ref/src/Microsoft.AspNetCore.App.Ref.csproj
M	src/Framework/test/Microsoft.AspNetCore.App.UnitTests.csproj
M	src/Framework/test/TargetingPackTests.cs
M	src/Installers/Debian/TargetingPack/Debian.TargetingPack.debproj
M	src/Installers/Rpm/Directory.Build.targets
M	src/Installers/Rpm/TargetingPack/Rpm.TargetingPack.rpmproj
M	src/SiteExtensions/LoggingBranch/LB.csproj
Falling back to patching base and 3-way merge...
Auto-merging src/SiteExtensions/LoggingBranch/LB.csproj
CONFLICT (content): Merge conflict in src/SiteExtensions/LoggingBranch/LB.csproj
Auto-merging src/Installers/Rpm/TargetingPack/Rpm.TargetingPack.rpmproj
Auto-merging src/Installers/Rpm/Directory.Build.targets
Auto-merging src/Installers/Debian/TargetingPack/Debian.TargetingPack.debproj
Auto-merging src/Framework/test/TargetingPackTests.cs
CONFLICT (content): Merge conflict in src/Framework/test/TargetingPackTests.cs
Auto-merging src/Framework/test/Microsoft.AspNetCore.App.UnitTests.csproj
CONFLICT (content): Merge conflict in src/Framework/test/Microsoft.AspNetCore.App.UnitTests.csproj
Auto-merging src/Framework/App.Ref/src/Microsoft.AspNetCore.App.Ref.csproj
Auto-merging eng/targets/ResolveReferences.targets
Auto-merging eng/Versions.props
CONFLICT (content): Merge conflict in eng/Versions.props
Auto-merging Directory.Build.targets
Auto-merging Directory.Build.props
CONFLICT (content): Merge conflict in Directory.Build.props
error: Failed to merge in the changes.
hint: Use 'git am --show-current-patch=diff' to see the failed patch
Patch failed at 0001 Always build App.Ref and the targeting packs - remove `$(IsTargetingPackBuilding)` entirely
When you have resolved this problem, run "git am --continue".
If you prefer to skip this patch, run "git am --skip" instead.
To restore the original branch and stop patching, run "git am --abort".
Error: The process '/usr/bin/git' failed with exit code 128

Please backport manually!

@dougbu dougbu requested a review from a team January 19, 2022 01:00
@dougbu dougbu marked this pull request as ready for review January 19, 2022 01:03
@dougbu dougbu requested a review from Pilchie as a code owner January 19, 2022 01:03
@RussKie
Copy link
Member

RussKie commented Jan 19, 2022

How do we verify API surface isn't changed between the current ref pack (e.g. X.Y.3) and the original ref pack (X.Y.0). @ericstj mentioned we should do an APICompat safeguard (dotnet/windowsdesktop#2362 (comment)).

@dougbu
Copy link
Contributor Author

dougbu commented Jan 19, 2022

How do we verify API surface isn't changed between the current ref pack (e.g. X.Y.3) and the original ref pack (X.Y.0).

APICompat is one way. Other options include exercising extreme diligence when reviewing servicing code changes (not the surest choice) and using Microsoft.CodeAnalysis.PublicApiAnalyzers in your builds (what we do in aspnetcore).

@RussKie
Copy link
Member

RussKie commented Jan 19, 2022

Other options include exercising extreme diligence when reviewing servicing code changes (not the surest choice)

If all fails, yes. Hahah :)

...and using Microsoft.CodeAnalysis.PublicApiAnalyzers in your builds (what we do in aspnetcore).

We use these too in Windows Forms, but I don't think these are used in WPF.

@dougbu dougbu merged commit 08968dd into dotnet:main Jan 19, 2022
@dougbu dougbu deleted the dougbu/istargetingpackbuilding.yes.39471 branch January 19, 2022 18:01
@ghost ghost added this to the 7.0-preview1 milestone Jan 19, 2022
@ericstj
Copy link
Member

ericstj commented Jan 19, 2022

APICompat is pretty easy to setup. Here's a sample of manual run. https://github.com/dotnet/runtime/blob/6de5c5ba25886270bddfbd827b6cf0b7f8a178a2/src/libraries/shims/ApiCompat.proj#L103

Basically you want to set up the latest build as the "contract" and the GA build as that "implementation". That ensures that anyone who compiles against surface area in the latest will have their references satisified by the GA build. You can use a PackageDownload to ensure you pull down the GA build.

@ghost
Copy link

ghost commented Jan 19, 2022

Hi @ericstj. It looks like you just commented on a closed PR. The team will most probably miss it. If you'd like to bring something important up to their attention, consider filing a new issue and add enough details to build context.

ShreyasJejurkar pushed a commit to ShreyasJejurkar/aspnetcore that referenced this pull request Jan 22, 2022
- remove `$(IsTargetingPackBuilding)` entirely
dougbu added a commit to dougbu/aspnetcore that referenced this pull request Jan 25, 2022
- remove `$(IsTargetingPackBuilding)` entirely
- 6.0.x version of dotnet#39568
wtgodbe pushed a commit to vseanreesermsft/aspnetcore that referenced this pull request Feb 1, 2022
- remove `$(IsTargetingPackBuilding)` entirely
- 6.0.x version of dotnet#39568
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants