Skip to content

Conversation

@oleksandr-didyk
Copy link
Contributor

Overview

Resolves dotnet/source-build#2655

This PR adds tooling for generating text-only packages. The entrypoint for the tooling is the newly added ./generate-text-packs.sh script, that allows the user to provide a package name, multiple package versions and multiple additional NuGet feeds to restore the package from.

The tooling is a modified version of similar MSBuild logic from dotnet/installer (source)

The tool was also used to re-generate almost all of the currently present text-only packages

Other changes

  • renamed ./generate.sh to ./generate-ref-packs.sh to better represent its functionality and not collide with the newly added ./generate-text-packs.sh
  • added missing *.csproj files for Microsoft.CodeAnalysis.Collections and Microsoft.NetCore.Platform
  • replaced existing LICENSE files with dotnet_library_license (change coming from the NuGet packages themselves)
  • updated <licenseUrl> in *.nuspec (change coming from the NuGet packages themselves)

Open questions

  • are the license changes valid or should they be reverted?
  • is the AllowedTextOnlyExtensions list valid or should be updated / removed completely? Originally copied from dotnet/installer with small additions of the following extensions: .-, .resx, .rtf and .xlf
  • existing *.nuspec files differ from those available in the NuGet packages by a few lines specified below. Is this something that should be made part of the tooling or could be added manually by the user?
  <files>
    <file src="*/**" target="/" exclude="obj/**/*.*" />
    <file src="*" target="/" />
  </files>

@omajid
Copy link
Member

omajid commented Aug 17, 2022

are the license changes valid or should they be reverted?

If the licenses are actually proprietary, that makes it impossible for open source projects to consume this repo and source-build. What would re-licensing look like?

fi

# -p:RestoreAdditionalProjectSources -> specifies additional Nuget package sources, including feeds: https://docs.microsoft.com/en-us/nuget/reference/msbuild-targets#restore-properties
# -p:RestoreUsingNuGetTargets -> switch that controls who will restore the project; if set to `false`, Arcade's Build.proj will execute the Restore target of the project itself.
Copy link
Member

Choose a reason for hiding this comment

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

Nice discovery!

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from ce62b9d to c23c83c Compare August 17, 2022 14:22
@oleksandr-didyk
Copy link
Contributor Author

Update - incorporating PR feedback:

  • removed reference to dotnet/installer in target comment
  • fixed spacing and formatting in PackageGenerator.proj

@crummel
Copy link
Contributor

crummel commented Aug 19, 2022

are the license changes valid or should they be reverted?

If the licenses are actually proprietary, that makes it impossible for open source projects to consume this repo and source-build. What would re-licensing look like?

I believe Dan S went and got permission to put these under the licenses as currently checked-in so we shouldn't revert. @leecow can probably confirm.

@MichaelSimons
Copy link
Member

I believe Dan S went and got permission to put these under the licenses as currently checked-in so we shouldn't revert. @leecow can probably confirm.

I confirmed this with Dan S.

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from c23c83c to 92cac66 Compare September 5, 2022 14:58
@oleksandr-didyk
Copy link
Contributor Author

Update - incorporating PR feedback:

  • removed changes to package licenses
  • unified generator scripts
  • extracted ref-pack restore logic and re-used it for text-only packages
  • minor improvements to the ./generate.sh script

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from 92cac66 to f53bf43 Compare September 6, 2022 08:42
@oleksandr-didyk
Copy link
Contributor Author

Update - incorporating PR feedback:

  • fixed typos
  • fixed formatting

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from f53bf43 to 1b482a2 Compare September 6, 2022 10:58
Copy link
Member

@MichaelSimons MichaelSimons left a comment

Choose a reason for hiding this comment

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

Changes look nice. I just have a couple comments.

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from 1b482a2 to 9f011d3 Compare September 9, 2022 13:34
@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from 9f011d3 to 0b498d7 Compare September 9, 2022 13:45
@MichaelSimons
Copy link
Member

@oleksandr-didyk, When making changes in response to code review feedback, I find it beneficial to preserve the individual commits. It helps the code reviewers see what was changed versus having to re-review the entire set of changes. The commits can be squashed via GitHub when merging the PR.

@oleksandr-didyk oleksandr-didyk force-pushed the feat-add-text-pack-generator branch from 0b498d7 to b551ba3 Compare September 12, 2022 11:47
@oleksandr-didyk oleksandr-didyk merged commit 37ccc2c into dotnet:main Sep 13, 2022
@oleksandr-didyk oleksandr-didyk deleted the feat-add-text-pack-generator branch September 13, 2022 11:58
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.

Add SBRP tooling to support text-only packages

6 participants