Skip to content
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

Fix missing readme entry in NuSpecs #4776

Merged
merged 1 commit into from
Dec 4, 2023
Merged

Conversation

RussKie
Copy link
Member

@RussKie RussKie commented Dec 1, 2023

Resolves #4722

$(IsShipping) is being set in one of the *.targets, which requires the readme setting to be done in a *.targets as well.

While at it, move the definitions to Packaging.targets which is the better suited location.

Microsoft Reviewers: Open in CodeFlow

Resolves dotnet#4722

`$(IsShipping)` is being set in one of the *.targets, which requires the
readme setting to be done in a *.targets as well.

While at it, move the definitions to Packaging.targets which is the
better suited location.
@ghost ghost assigned RussKie Dec 1, 2023
@RussKie RussKie requested a review from joperezr December 1, 2023 06:21
Copy link
Member

@joperezr joperezr left a comment

Choose a reason for hiding this comment

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

While this approach works too, Is there any issue of instead removing the condition entirely for PackageReadmeFile, and just have the one single package that violates this (the transport docs package) to either override the property or generate an empty README? Given property and item ordering, and how these ones being moved control many other things, I think that doing it that way would reduce the risk of this introducing a different hard-to-catch issue.

When it comes to MSBuild and import ordering, I usually try to pick the less risky change given it's hard to catch potential regressions with these changes.

@joperezr
Copy link
Member

joperezr commented Dec 1, 2023

If you still think that this is the right way to address the issue, I'm fine with it too, I would just ask that before merging we do a full diff of package contents and nuspec before and after the change, to ensure that we are getting the desired effect.

@RussKie
Copy link
Member Author

RussKie commented Dec 4, 2023

This is the correct way to fix the issue.

Before (the latest main):

image

After (private build of this PR):

image

@RussKie
Copy link
Member Author

RussKie commented Dec 4, 2023

Btw, if there's a desire to open the readmes in VS upon install, then those have to be readme.txt and not readme.md (source).

The markdown readmes can be explored via the Solution Explorer > Depenendecies:
image

@joperezr
Copy link
Member

joperezr commented Dec 4, 2023

This is the correct way to fix the issue.

I didn't mean to infer that your change doesn't work, I do believe that it works (and thanks for validating that 😃). I was just suggesting to consider a less-risky change that would also work which was keep properties where they were, and just removing the condition as it isn't necessary (once we add a dummy readme to the docs package). But anyway, I don't think this feedback is worth blocking this PR, so I'll go ahead and approve, and let you decide if you want to go with the less-risky alternative.

@RussKie RussKie merged commit ca03b0c into dotnet:main Dec 4, 2023
6 checks passed
@ghost ghost added this to the 8.1 milestone Dec 4, 2023
@RussKie RussKie deleted the fix_4722 branch December 4, 2023 21:39
@github-actions github-actions bot locked and limited conversation to collaborators Jan 4, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

READMEs are not rendered on NuGet.org
3 participants