Skip to content

Add resources to our NuGet packages#26593

Merged
tmeschter merged 3 commits intodotnet:masterfrom
tmeschter:AddResourcesToNuGetPackages-180502
May 4, 2018
Merged

Add resources to our NuGet packages#26593
tmeschter merged 3 commits intodotnet:masterfrom
tmeschter:AddResourcesToNuGetPackages-180502

Conversation

@tmeschter
Copy link
Copy Markdown
Contributor

Now that we're producing satellite assemblies with localized resources
directly out of the repo we can include them in our NuGet packages.

Not all of these packages currently have satellite assemblies, but if they are added later we will pick them up.

@tmeschter tmeschter requested a review from a team as a code owner May 3, 2018 17:31
@tmeschter tmeschter requested review from a team May 3, 2018 17:31
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Nit: is it better to keep these by the lines higher up so it's easier to see that each regular one has it's resources along with?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

It's a source only package -- should never have these.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Oh yeah, meant to revert that one.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Not strictly necessary, since in this case this package is really a reference-assembly only package -- we could technically put the binary asset into ref/ and it'd be fine. But this is fine if you prefer the consistency.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

In that case I'll just revert this file. There's no real point in including the satellite assemblies if we don't mean for this package to be used for redistribution.

@tmeschter tmeschter force-pushed the AddResourcesToNuGetPackages-180502 branch from 5182a2e to 8d9e2c0 Compare May 3, 2018 17:46
Now that we're producing satellite assemblies with localized resources
directly out of the repo we can include them in our NuGet packages.
@tmeschter tmeschter force-pushed the AddResourcesToNuGetPackages-180502 branch from 8d9e2c0 to 3a9f937 Compare May 3, 2018 17:53
We don't really need resource assemblies in our VS compiler toolset
insertion, so just ignore them rather than making
Roslyn.BuildDevDivInsertionFiles.exe understand them properly.
@tmeschter tmeschter force-pushed the AddResourcesToNuGetPackages-180502 branch from 68f7964 to a6c76cc Compare May 3, 2018 23:00
Update the entry for
Microsoft.CodeAnalysis.EditorFeatures.Wpf.resources.dll to actually
include the "resources" part of the file name.
@tmeschter tmeschter requested a review from jaredpar May 4, 2018 18:37
@tmeschter
Copy link
Copy Markdown
Contributor Author

@jasonmalinowski @jaredpar I've had to make some changes to DevDivInsertionFiles; you may want to take a look again.

Copy link
Copy Markdown
Member

@jasonmalinowski jasonmalinowski left a comment

Choose a reason for hiding this comment

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

New changes still look good, but is there a reason to not include the localized versions in the Razzle toolset? I'd like to think as a global company people get benefit from that but maybe not?

<file src="$thirdPartyNoticesPath$" target="" />

<!-- Satellite assemblies -->
<file src="Dlls\CodeAnalysis\**\Microsoft.CodeAnalysis.resources.dll" target="lib\netstandard1.3" />
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

**\ [](start = 33, length = 3)

Assume the directory structure signified by ** is preserved under the target directory?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Correct.

@tmeschter
Copy link
Copy Markdown
Contributor Author

tmeschter commented May 4, 2018

@jasonmalinowski Right now the DevDivInsertionFiles tooling doesn't know how to expand the wildcards in signtool.json and the .nuspec files. So it can see that those files include .resource.dll files, but it can't actually find them on disk. We don't need to fix that (certainly not for the immediate purpose of getting the resource assemblies into the nuget packages) so I'd rather just avoid the issue.

Copy link
Copy Markdown
Member

@jaredpar jaredpar left a comment

Choose a reason for hiding this comment

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

:shipit:

@tmeschter tmeschter merged commit 990fbbb into dotnet:master May 4, 2018
@tmeschter tmeschter deleted the AddResourcesToNuGetPackages-180502 branch May 4, 2018 21:32
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.

3 participants