Skip to content

[release/6.0] Always build App.Ref and the targeting packs #39620

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

Conversation

dougbu
Copy link
Member

@dougbu dougbu commented Jan 19, 2022

[release/6.0] Always build App.Ref and the targeting packs

Description

Remove $(IsTargetingPackBuilding) settings and remove all use of the property. For example, it's not necessary to reference Microsoft.AspNetCore.App.Runtime.csproj when Microsoft.AspNetCore.App.Ref.csproj won't build; references to Microsoft.AspNetCore.App.Ref.csproj are all we need.

In detail:

  • where Conditions were positive ($(IsTargetingPackBuilding) or $(IsTargetingPackBuilding)' == 'true'), remove the Condition or its relevant portion
  • otherwise, (! $(IsTargetingPackBuilding) or $(IsTargetingPackBuilding)' != 'true'), remove the affected block or the relevant portion of the Condition

nit: simplify any leftover Conditions if possible

Fixes #39471

Customer Impact

Without this change, we're unable to create source-build tarballs except with significant hacks.

We also realized skipping builds of the …Ref packages and targeting pack installers would be uncommon given their new analyzer and code generator content in 6.0.x. Decided always building these assets simplified everything in the relevant repos.

In this repo, not releasing the targeting packs was a major exception. We build and pack everything else for every release and have since 3.1.0.

Regression?

  • Yes
  • No

The main difference from 5.0.x in this release is the analyzer and code generator assemblies in our targeting packs. This content makes it easier to justify building the targeting packs for every release.

Risk

  • High
  • Medium
  • Low

This PR removes some complexity from our build and should simplify our release process.

Verification

  • Manual (required)
  • Automated

Packaging changes reviewed?

  • Yes
  • No
  • N/A

This PR does not change individual packages. It just ensures Microsoft.AspNetCore.App.Ref.nupkg and our targeting pack installers are always created.


When servicing release/2.1

  • Make necessary changes in eng/PatchConfig.props

@dougbu dougbu requested a review from a team January 19, 2022 01:14
@dougbu dougbu requested a review from Pilchie as a code owner January 19, 2022 01:14
@ghost ghost added this to the 6.0.x milestone Jan 19, 2022
@ghost
Copy link

ghost commented Jan 19, 2022

Hi @dougbu. If this is not a tell-mode PR, please make sure to follow the instructions laid out in the servicing process document.
Otherwise, please add tell-mode label.

@dougbu dougbu added the area-infrastructure Includes: MSBuild projects/targets, build scripts, CI, Installers and shared framework label Jan 19, 2022
@dougbu
Copy link
Member Author

dougbu commented Jan 19, 2022

@Pilchie should we treat this one as ask-mode due to the release implications i.e. shipping more❔

@Pilchie
Copy link
Member

Pilchie commented Jan 19, 2022

I can bring it up in Tactics tomorrow.

@dougbu
Copy link
Member Author

dougbu commented Jan 19, 2022

I can bring it up in Tactics tomorrow.

Thanks @Pilchie. I added the servicing template to the description.

@dougbu dougbu added the Servicing-consider Shiproom approval is required for the issue label Jan 19, 2022
@ghost
Copy link

ghost commented Jan 19, 2022

Hi @dougbu. Please make sure you've updated the PR description to use the Shiproom Template. Also, make sure this PR is not marked as a draft and is ready-to-merge.

To learn more about how to prepare a servicing PR click here.

@leecow
Copy link
Member

leecow commented Jan 20, 2022

Need to confirm we're doing the right thing on update and uninstall.

@leecow leecow added Servicing-approved Shiproom has approved the issue and removed Servicing-consider Shiproom approval is required for the issue labels Jan 20, 2022
@leecow leecow modified the milestones: 6.0.x, 6.0.3 Jan 20, 2022
@dougbu dougbu force-pushed the dougbu/istargetingpackbuilding.yes.39471.60x branch from e764dbd to f1cb9ec Compare January 25, 2022 00:54
@dougbu
Copy link
Member Author

dougbu commented Jan 25, 2022

Rebased on brecon/60cookie to confirm continued success

@TanayParikh
Copy link
Contributor

@dougbu can this be merged now given the CI is passing and it is servicing approved?

@wtgodbe
Copy link
Member

wtgodbe commented Feb 1, 2022

Closing in favor of #39924

@wtgodbe wtgodbe closed this Feb 1, 2022
@dougbu
Copy link
Member Author

dougbu commented Feb 1, 2022

can this be merged now given the CI is passing and it is servicing approved?

@TanayParikh /fyi what @wtgodbe did is a bit cleaner because the change will go in together w/ the new branding. We considered force-merging the branding PRs then doing these but either works. And both are better than doing a lot of stuff before branding is done.

@dougbu dougbu deleted the dougbu/istargetingpackbuilding.yes.39471.60x branch February 1, 2022 23:24
@dougbu dougbu removed this from the 6.0.3 milestone Mar 7, 2022
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 Servicing-approved Shiproom has approved the issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants