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.

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

fix pack issue with readmes and snupkgs #4268

Merged
merged 6 commits into from
Sep 21, 2021
Merged

fix pack issue with readmes and snupkgs #4268

merged 6 commits into from
Sep 21, 2021

Conversation

zkat
Copy link
Contributor

@zkat zkat commented Sep 15, 2021 •
edited by heng-liu
Loading

Bug

Fixes: NuGet/Home#10791

Regression? No

Description

According to NuGet.org symbol package constraints, the symbol package should not have the readme file.
So this PR:

  1. skip validation of readme file if the package is a symbol package
  2. add a test for packing snupkgs with readme file, and verify it's successfully created the snupkgs.

PR Checklist

  • PR has a meaningful title

  • PR has a linked issue.

  • Described changes

  • Tests

    • Automated tests added
    • OR
    • Test exception
    • OR
    • N/A

  • Documentation

    • Documentation PR or issue filled
    • OR
    • N/A

@zkat zkat requested a review from a team as a code owner September 15, 2021 22:20
jeffkl
jeffkl reviewed Sep 16, 2021
View reviewed changes
src/NuGet.Core/NuGet.Common/PathUtil/PathResolver.cs Outdated
basePath = Path.Combine(basePath, parentDirectoryPath);
searchPath = searchPath.Substring(parentDirectoryPath.Length);
}
// while (searchPath.StartsWith(parentDirectoryPath, StringComparison.OrdinalIgnoreCase))
Copy link
Contributor

Choose a reason for hiding this comment

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

I'm not sure why this is commented out?

Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks for reviewing! Reverted.

@jeffkl
Copy link
Contributor

jeffkl commented Sep 16, 2021

We need a unit test that would have caught the fact that the functionality is broken.

@heng-liu
Copy link
Contributor

We need a unit test that would have caught the fact that the functionality is broken.

Thanks! Added.

jeffkl
jeffkl approved these changes Sep 16, 2021
View reviewed changes
Copy link
Contributor

@jeffkl jeffkl left a comment

Choose a reason for hiding this comment

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

🥇

@heng-liu
Copy link
Contributor

Hi @aortiz-msft , pls approve this PR if it's good to merge. Thanks!

@zivkan
Copy link
Member

zivkan commented Sep 17, 2021 •
edited
Loading

Edit: Apparently I didn't get enough sleep. All's good.

@aortiz-msft aortiz-msft self-requested a review September 20, 2021 19:38
aortiz-msft
aortiz-msft requested changes Sep 20, 2021
View reviewed changes
Copy link
Contributor

@aortiz-msft aortiz-msft left a comment

Choose a reason for hiding this comment

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

Could you please the PR description? I don't think that "This does the stuff over at NuGet/Home#10791 (comment)" is entirely accurate.

@heng-liu
Copy link
Contributor

Hi @aortiz-msft , thanks for the suggestion! Updated the description in the PR.

@heng-liu heng-liu merged commit e685bdb into dev Sep 21, 2021
@heng-liu heng-liu deleted the zkat/fix-10791 branch September 21, 2021 16:53
@RehanSaeed
Copy link

Any ideas when this will roll out? .NET Core 6?

@loic-sharma
Copy link
Contributor

This should be in .NET 6 RC2 which will be released in a few weeks.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@heng-liu heng-liu heng-liu left review comments

@loic-sharma loic-sharma loic-sharma approved these changes

@nkolev92 nkolev92 nkolev92 approved these changes

@jeffkl jeffkl jeffkl approved these changes

@donnie-msft donnie-msft donnie-msft approved these changes

@aortiz-msft aortiz-msft aortiz-msft approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Cannot use embeded PackageReadmeFile when using snupkg for symbols
9 participants
@zkat @jeffkl @heng-liu @zivkan @RehanSaeed @loic-sharma @nkolev92 @donnie-msft @aortiz-msft