Skip to content

Mark AspNetCore projects that aren't packaged explicitly #18767

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 4 commits into from
Feb 13, 2020
Merged

Mark AspNetCore projects that aren't packaged explicitly #18767

merged 4 commits into from
Feb 13, 2020

Conversation

@dougbu
Copy link
Contributor

@dougbu dougbu commented Feb 3, 2020

  • avoid NU5104 warnings due to confusing versioning
  • $(IsShippingPackage) was semantically incorrect in any case

@dougbu
Mark AspNetCore projects that aren't packaged explicitly
421f281 
- avoid NU5104 warnings due to confusing versioning
- `$(IsShippingPackage)` was semantically incorrect in any case
@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 Feb 3, 2020
@dougbu dougbu added this to the 3.1.3 milestone Feb 3, 2020
@dougbu dougbu requested a review from a team February 3, 2020 23:50
@dougbu
Copy link
Contributor Author

dougbu commented Feb 3, 2020

@Pilchie I'm proposing this as tell-mode cleanup for 3.1.3 because the versions in AspNetCore.App projects that don't ship as packages aren't used -- they just cause 30 or so NU5104 without this change. For example, our custom RemoveSharedFrameworkDependencies task removes SharedFx / non-package dependencies regardless of the dependency's version.

@wtgodbe
Copy link
Member

wtgodbe commented Feb 4, 2020

Any corresponding cleanup needed in Directory.Build.targets, where we used to calculate IsPackable based off of IsShippingPackage?

@Pilchie
Copy link
Member

Pilchie commented Feb 4, 2020

Sounds good to me.

@wtgodbe
Copy link
Member

wtgodbe commented Feb 4, 2020

Or maybe we should change https://github.com/dotnet/aspnetcore/blob/release/3.1/Directory.Build.targets#L8 to only set this property if it isn't already set

@dougbu
Copy link
Contributor Author

dougbu commented Feb 4, 2020

Or maybe we should change https://github.com/dotnet/aspnetcore/blob/release/3.1/Directory.Build.targets#L8 to only set this property if it isn't already set

With this PR, $(IsPackable) will only be set if $(IsShippingPackage) is not set explicitly for an AspNetCore.App project. It's a no-op because we're explicit about that too i.e.

<IsAspNetCoreApp>true</IsAspNetCoreApp>
<IsShippingPackage>true</IsShippingPackage>

appears in all of the package+SharedFx projects. I left the line around to provide a reasonable default if someone leaves both <IsShippingPackage>true</IsShippingPackage> and <IsPackable>false</IsPackable> out in 'master' (once this merges forward). Any objections?

dougbu added 3 commits February 7, 2020 14:37
@dougbu
Remove redundant $(IsShippingPackage) settings in `$(IsAspNetCoreAp…
909bb33 
…p)` projects

- default is `true` for all implementation projects
@dougbu
Use $(IsPackable) when deciding how $(IsAspNetCoreApp) projects a…
4d92118 
…re handled

- remove all use of `$(IsShippingPackage)` for shared framework composition
- update documentation to match these changes

nits:
- remove odd default for `$(IsPackable)` in Directory.Build.targets
  - no longer relevant since all `$(IsAspNetCoreApp)` projects are `$(IsShippingPackage)` too
- include more information in docs/ProjectProperties.md
@dougbu
Add direct System.Text.Json references
ae7e34f 
- avoid MSB3277 warnings
@dougbu dougbu force-pushed the dougbu/cleanup.warnings branch from dcda109 to ae7e34f Compare February 10, 2020 00:22
@dougbu
Copy link
Contributor Author

dougbu commented Feb 10, 2020

@wtgodbe lease give this PR another once-over.

I changed this a fair amount since your approval. It's now building successfully 😺 and removes NU1504 errors except the one @JunTaoLuo fixed in #18790 as well as all MSB3277 warnings and some redundancy.

wtgodbe
wtgodbe approved these changes Feb 10, 2020
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.

(Still) looks good

@analogrelay analogrelay added tell-mode Indicates a PR which is being merged during tell-mode and removed tell-mode Indicates a PR which is being merged during tell-mode labels Feb 13, 2020
@dougbu
Copy link
Contributor Author

dougbu commented Feb 13, 2020

@wtgodbe I see branding is done; any reason I should wait on this?

@wtgodbe wtgodbe merged commit a6c43b1 into release/3.1 Feb 13, 2020
@wtgodbe wtgodbe deleted the dougbu/cleanup.warnings branch February 13, 2020 23:51
@wtgodbe wtgodbe mentioned this pull request Feb 14, 2020
Merged
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

@analogrelay analogrelay Awaiting requested review from analogrelay

@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

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

+1 more reviewer

@JunTaoLuo JunTaoLuo JunTaoLuo approved these changes

Reviewers whose approvals may not affect merge requirements

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.3

Development

Successfully merging this pull request may close these issues.

6 participants

@dougbu @wtgodbe @Pilchie @JunTaoLuo @analogrelay