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

Split library and package Readme #78888

Merged
merged 11 commits into from
Dec 12, 2022
Merged

Conversation

MSDN-WhiteKnight
Copy link
Contributor

I've initially submitted library and package Readmes as single file for Microsoft.Extension.Configuration libraries, but maintainers raised issue that this exposes unneeded information on NuGet gallery: #59630 (comment)

So the new convention is:

  • README.md - Library Readme. Contains documentation for contributors, and very little info for users.
  • PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

/cc @jeffhandley @ViktorHofer

Related to: #59630, #77733

@ghost ghost added the community-contribution Indicates that the PR has been added by a community member label Nov 27, 2022
@ghost
Copy link

ghost commented Nov 27, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

I've initially submitted library and package Readmes as single file for Microsoft.Extension.Configuration libraries, but maintainers raised issue that this exposes unneeded information on NuGet gallery: #59630 (comment)

So the new convention is:

  • README.md - Library Readme. Contains documentation for contributors, and very little info for users.
  • PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

/cc @jeffhandley @ViktorHofer

Related to: #59630, #77733

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Nov 27, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

I've initially submitted library and package Readmes as single file for Microsoft.Extension.Configuration libraries, but maintainers raised issue that this exposes unneeded information on NuGet gallery: #59630 (comment)

So the new convention is:

  • README.md - Library Readme. Contains documentation for contributors, and very little info for users.
  • PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

/cc @jeffhandley @ViktorHofer

Related to: #59630, #77733

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-Meta, community-contribution

Milestone: -

@ghost
Copy link

ghost commented Nov 27, 2022

Tagging subscribers to this area: @dotnet/area-extensions-configuration
See info in area-owners.md if you want to be subscribed.

Issue Details

I've initially submitted library and package Readmes as single file for Microsoft.Extension.Configuration libraries, but maintainers raised issue that this exposes unneeded information on NuGet gallery: #59630 (comment)

So the new convention is:

  • README.md - Library Readme. Contains documentation for contributors, and very little info for users.
  • PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

/cc @jeffhandley @ViktorHofer

Related to: #59630, #77733

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-Extensions-Configuration, community-contribution

Milestone: -

@tarekgh tarekgh added this to the 8.0.0 milestone Nov 27, 2022
@ghost
Copy link

ghost commented Nov 28, 2022

Tagging subscribers to this area: @dotnet/area-meta
See info in area-owners.md if you want to be subscribed.

Issue Details

I've initially submitted library and package Readmes as single file for Microsoft.Extension.Configuration libraries, but maintainers raised issue that this exposes unneeded information on NuGet gallery: #59630 (comment)

So the new convention is:

  • README.md - Library Readme. Contains documentation for contributors, and very little info for users.
  • PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

/cc @jeffhandley @ViktorHofer

Related to: #59630, #77733

Author: MSDN-WhiteKnight
Assignees: -
Labels:

area-Meta, community-contribution

Milestone: 8.0.0

@ViktorHofer
Copy link
Member

@MSDN-WhiteKnight I hope it's OK that I pushed to your branch. I made the following refinements (three commits):

  1. Automatically include the package readme file in the package if it exists. That reduces duplicity in every project file.
  2. Consistently rename all packaging README.md files to PACKAGE.md.
  3. Move all PACKAGE.md files into the "src" sub directory as the other subdirectories (ref, tests, ...) are non-shipping. Moving the PACKAGE.md file into the src directory makes it clear that the information applies to the source assembly and its package only.

@jkotas
Copy link
Member

jkotas commented Nov 28, 2022

PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

Does NuGet gallery look for PACKAGE.md? Or do we need to rename it to README.md as part of packing?

@MSDN-WhiteKnight
Copy link
Contributor Author

PACKAGE.md - Package Readme. Contains documentation for users (this one is published on NuGet gallery).

Does NuGet gallery look for PACKAGE.md? Or do we need to rename it to README.md as part of packing?

NuGet uses file specified by PackageReadmeFile property. This property is now globally set in packaging.targets to PACKAGE.md so yes, it will be recognized.

@@ -57,6 +58,11 @@
TargetFramework="$(NetFrameworkMinimum)" />
</ItemGroup>

<!-- Add a package README file from. -->
<ItemGroup Condition="'$(PackageReadmeFile)' != ''">
<None Include="$(PackageReadmeFile)" Pack="true" PackagePath="\" />
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
<None Include="$(PackageReadmeFile)" Pack="true" PackagePath="\" />
<None Include="$(PackageReadmeFile)" Pack="true" PackagePath="README.md" />

You want the file to be named README.md, right?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We don't have to specify name README.md, NuGet will recognize any name set by PackageReadmeFile property. Also PackagePath seems to be a path without name, it is used like that in docs: https://learn.microsoft.com/en-us/nuget/reference/msbuild-targets#packagereadmefile

@@ -11,6 +11,7 @@
<!-- Don't include target platform specific dependencies, since we use the target platform to represent RIDs instead -->
<SuppressDependenciesWhenPacking Condition="'$(TargetPlatformIdentifier)' != ''">true</SuppressDependenciesWhenPacking>
<PackageDesignerMarkerFile>$(MSBuildThisFileDirectory)useSharedDesignerContext.txt</PackageDesignerMarkerFile>
<PackageReadmeFile Condition="'$(PackageReadmeFile)' == '' and Exists('PACKAGE.md')">PACKAGE.md</PackageReadmeFile>
Copy link
Member

Choose a reason for hiding this comment

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

Maybe the file name could be PACKAGE_README.md? PACKAGE.md doesn't really describe exactly what the file is.

Copy link
Member

Choose a reason for hiding this comment

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

When it comes to NuGet Gallery, the PACKAGE.md file is automatically respected and displayed as the package's nuspec encodes it. Personally, I'm fine with the PACKAGE.md name and I don't think it's common that a dev crack opens a nuget package in order to understand how to use it.

Copy link
Member

Choose a reason for hiding this comment

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

It's more about readability/understandability of the files in the repo.

Copy link
Member

Choose a reason for hiding this comment

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

Other options include:

  1. PackageReadme.md
  2. The file under a docs folder, and still name it README.md. e.g. src/docs/README.md
  3. Since the files are split already - one is in the root of the library, and one is under src - they could both be named README.md.

Copy link
Contributor Author

@MSDN-WhiteKnight MSDN-WhiteKnight Nov 28, 2022

Choose a reason for hiding this comment

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

The PACKAGE.md choice was based on this suggestion: #59630 (comment) I don't think this name needs to be long as there's not a lot of .md files in folder to disambiguate between them. So personally i'd prefer either README.md or PACKAGE.md.

Copy link
Member

@ViktorHofer ViktorHofer Nov 28, 2022

Choose a reason for hiding this comment

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

Being able to distinguish between a package documentation file and a repo specific contribution guidance seems valuable to me. We unintentionally started shipping the contribution guidance files to customers and this PR fixes that: #59630 (comment).

<!-- This library has been annotated to be AOT safe -->
<EnableAOTAnalyzer>true</EnableAOTAnalyzer>
<IsPackable>true</IsPackable>
Copy link
Member

Choose a reason for hiding this comment

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

Why is this moved/changed? Seems unnecessary.

Copy link
Member

Choose a reason for hiding this comment

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

It was moved to group packaging properties together.

@ViktorHofer
Copy link
Member

@MSDN-WhiteKnight can you please update https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/libraries-packaging.md and add a section that describes the new conventions (including mentioning/explaining that the file is automatically included in the package if conventions are followed)?

@MSDN-WhiteKnight
Copy link
Contributor Author

can you please update https://github.com/dotnet/runtime/blob/main/docs/coding-guidelines/libraries-packaging.md and add a section that describes the new conventions (including mentioning/explaining that the file is automatically included in the package if conventions are followed)?

I've added a section about Package readme, and also edited PackageDescription guidelines to remove the list of entry-point types (it is now obsolete as including it in readme does it better). Should i also explain what is library readme and how it's different from package readme? In libraries-packaging.md, or in some other higher-level document?

@ViktorHofer
Copy link
Member

I've added a section about Package readme, and also edited PackageDescription guidelines to remove the list of entry-point types (it is now obsolete as including it in readme does it better). Should i also explain what is library readme and how it's different from package readme? In libraries-packaging.md, or in some other higher-level document?

Thanks a lot. Regarding libraries README files, those are already documented in the src/libraries/README.md file: https://github.com/dotnet/runtime/blob/main/src/libraries/README.md

@ViktorHofer ViktorHofer merged commit 479d013 into dotnet:main Dec 12, 2022
@MSDN-WhiteKnight MSDN-WhiteKnight deleted the package-readme branch December 12, 2022 11:14
@ghost ghost locked as resolved and limited conversation to collaborators Jan 11, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
area-Meta community-contribution Indicates that the PR has been added by a community member
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants