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

Update NuGet Package Content and Metadata #143

Merged
merged 4 commits into from Jan 28, 2021

Conversation

wsugarman
Copy link
Member

@wsugarman wsugarman commented Jan 25, 2021

Description

The NuGet packages are missing some important metadata that would make development easier, and some of the dependencies are incorrect. This PR sets out to address the following:

  • Remove build and development dependencies from NuGet's metadata (Eg. consumers don't need the analyzers)
  • Add XML doc to .nupkg for intellisense
    • Fix malformed or invalid XML doc
  • Ensure symbol (*.pdb) files are included in .nupkg
  • Compile code as deterministic
    • Add CI build flag to CI builds
  • Update NuGet metadata
    • Author
    • Company
    • Copyright
    • Description
    • Product
  • Normalize .csproj identation to 2 spaces

Related issues

N/A

Testing

  • Re-ran tests
  • Check package metadata locally

<WarningsAsErrors />
<DebugType>Portable</DebugType>
<LangVersion>8.0</LangVersion>
<Authors>Microsot Health Team</Authors>
Copy link
Member Author

Choose a reason for hiding this comment

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

This value is freeform and doesn't need to be related to any user in the feed/nuget.org. Does anyone have another value they'd like to use here?

<HighEntropyVA>true</HighEntropyVA>
<EnableSourceLink Condition="$([MSBuild]::IsOSPlatform('osx'))">false</EnableSourceLink>
Copy link
Contributor

Choose a reason for hiding this comment

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

Does this work on OSX now?

Copy link
Member Author

Choose a reason for hiding this comment

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

Looking at SourceLink's issues page, the only potential issue I found was related to mono: dotnet/sourcelink#155. This has since been fixed, so I think we're good to go.

@@ -17,14 +17,14 @@ steps:
displayName: 'dotnet build $(buildConfiguration)'
inputs:
command: build
arguments: '--configuration $(buildConfiguration) /p:AssemblyVersion="$(assemblySemVer)" /p:FileVersion="$(assemblySemFileVer)" /p:InformationalVersion="$(informationalVersion)" /warnaserror'
arguments: '--configuration $(buildConfiguration) /p:AssemblyVersion="$(assemblySemVer)" /p:FileVersion="$(assemblySemFileVer)" /p:InformationalVersion="$(informationalVersion)" /p:ContinuousIntegrationBuild=true /warnaserror'
Copy link
Member Author

Choose a reason for hiding this comment

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

This flag sets determinism-related flags that would only apply on a CI server, like deterministic source paths, etc.

@@ -1,6 +1,7 @@
<Project Sdk="Microsoft.NET.Sdk">

<PropertyGroup>
<Description>Identity primitives used by Microsoft Health.</Description>
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe something like Identity primitives used by Microsoft Health for local development scenarios.

Copy link
Member Author

@wsugarman wsugarman Jan 28, 2021

Choose a reason for hiding this comment

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

Sounds good! Changed to Identity primitives used by Microsoft Health for local development.

<PackageReference Include="Newtonsoft.Json.Schema" Version="3.0.13" />
</ItemGroup>
<ItemGroup>
<PackageReference Include="Microsoft.Extensions.Hosting.Abstractions" Version="$(SdkPackageVersion)" />
Copy link
Member

Choose a reason for hiding this comment

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

Re: Normalize .csproj identation to 2 spaces
What is the default? Will this play nice with everyone's editors, is there a settings file to make this consistent?

Copy link
Member Author

Choose a reason for hiding this comment

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

It should, as this is the default (and most of the other files were using this). However, we may want to consider the use of an .editorconfig file in the near future, perhaps as part of another PR.

@@ -3,8 +3,6 @@
<PropertyGroup>
<TargetFrameworks>$(SupportedFrameworks);</TargetFrameworks>
<NoWarn>$(NoWarn);NU1603</NoWarn>
</PropertyGroup>
<PropertyGroup>
<DefineConstants>R5</DefineConstants>
Copy link
Member

Choose a reason for hiding this comment

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

I guess while we're here, is there anything that depends on <DefineConstants>R5</DefineConstants>? That was a concept we are using in the FHIR repo

Copy link
Member Author

Choose a reason for hiding this comment

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

I don't see it being used. I'll remove it

@wsugarman wsugarman merged commit fee483e into master Jan 28, 2021
@wsugarman wsugarman deleted the users/wsugarman/FixNuGetDef branch January 28, 2021 18:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants