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

Passing GeneratePathProperty metadata as part of nomination #4226

Open
wants to merge 4 commits into
base: dev15.9.x
from

Conversation

Projects
None yet
6 participants
@jainaashish
Contributor

jainaashish commented Nov 6, 2018

AS part of NuGet/NuGet.Client#2271 We added a new feature to generate msbuild property to nuget package path if someone set GeneratePathProperty on a PackageReference. But Project system wasn't updated to pass this new metadata to NuGet as part of nomination.

This PR is to pass that and pass this new metadata to NuGet.

@jainaashish jainaashish requested a review from dotnet/project-system as a code owner Nov 6, 2018

@jeffkl

jeffkl approved these changes Nov 6, 2018

@jeffkl

This comment has been minimized.

jeffkl commented Nov 6, 2018

@jainaashish it looks like you need to update the XLF

'ProjectSystem\Rules\xlf\ProjectReference.xaml.cs.xlf' is out-of-date with 'ProjectSystem\Rules\ProjectReference.xaml'. Run `msbuild /t:UpdateXlf` to update .xlf files or set UpdateXlfOnBuild=true to update them on every build
@@ -112,6 +112,11 @@
Description="Version of reference.">
</StringProperty>
<StringProperty Name="GeneratePathProperty"

This comment has been minimized.

@davkean

davkean Nov 6, 2018

Member

What does this represent? This name and description is not clear to me.

This comment has been minimized.

@jainaashish

jainaashish Nov 7, 2018

Contributor

this generates msbuild property Pkg<Package_Name> which points to the package root path. This is what I've tried to explain in description. Let me know if you've better text.

Generates msbuild property to the path of this reference

This comment has been minimized.

@davkean

davkean Nov 7, 2018

Member

"Indicates whether to generate an MSBuild property with the location of the package's root directory. The generated property name is in the form of 'Pkg[PackageID]', where '[PackageID]' is the ID of the package with any periods '.' replaced with underscores '_'."

@davkean

This change needs to be on PackageReference, not ProjectReference.

@@ -112,6 +112,11 @@
Description="Version of reference.">
</StringProperty>
<StringProperty Name="GeneratePathProperty"
Visible="True"
DisplayName="GeneratePathProperty"

This comment has been minimized.

@davkean

davkean Nov 6, 2018

Member

Don't specific DisplayName if its the same as name. Display Name should be "Generate path property", but it's still not very clear what that is or what it means.

@jainaashish

This comment has been minimized.

Contributor

jainaashish commented Nov 7, 2018

@davkean ProjectReference was updated by mistake so I've corrected that. I wasn't able to update XLF since msbuild /t:UpdateXlf or build.cmd both are failing to build stating missing settings. So is there other way to generate these?

@davkean

This comment has been minimized.

Member

davkean commented Nov 7, 2018

@jainaashish Type "build.cmd".

@davkean

This comment has been minimized.

Member

davkean commented Nov 7, 2018

This should also be <BoolProperty/> not <StringProperty.

@@ -85,6 +85,11 @@
DisplayName="NoWarn"
Description="Comma-delimited list of warnings that should be suppressed for this package" />
<StringProperty Name="GeneratePathProperty"
Visible="True"
DisplayName="Generate Path Property"

This comment has been minimized.

@davkean

davkean Nov 7, 2018

Member

"Generate path property" I know that PackageReference isn't consistent here, I've filed #4231 to fix this.

@jainaashish

This comment has been minimized.

Contributor

jainaashish commented Nov 7, 2018

This should also be <BoolProperty/> not <StringProperty.

What exactly is the diff? I saw some existing Boolean property like IsImplicitlyDefined are also defined as StringProperty

@davkean

This comment has been minimized.

Member

davkean commented Nov 7, 2018

It controls how it is persisted and displayed in the Properties window. IsImplicitlyDefined isn't displayed in the UI.

@davkean

davkean approved these changes Nov 7, 2018

@Pilchie

Pilchie approved these changes Nov 7, 2018

@@ -82,6 +82,16 @@
<target state="translated">Umístění odkazovaného balíčku</target>
<note />
</trans-unit>
<trans-unit id="BoolProperty|GeneratePathProperty|DisplayName">

This comment has been minimized.

@Pilchie

Pilchie Nov 7, 2018

Member

Tagging @tmeschter. Note that we don't currently have any more loc handbacks planned for 15.9, so this string would remain untranslated until Dev16.

This comment has been minimized.

@tmeschter

tmeschter Nov 8, 2018

Contributor

If the display name or description are going to be visible to users then we can't take this change. Loc cut-off was over a week ago.

@tmeschter

tmeschter requested changes Nov 8, 2018 edited

Blocking the PR as there appear to be user-visible loc changes, and we don't have time to resolve them on the current schedule.
We need to get positive confirmation from the director that we can ship with untranslated resources, or slip the schedule to accomodate another loc pass.

@@ -82,6 +82,16 @@
<target state="translated">Umístění odkazovaného balíčku</target>
<note />
</trans-unit>
<trans-unit id="BoolProperty|GeneratePathProperty|DisplayName">

This comment has been minimized.

@tmeschter

tmeschter Nov 8, 2018

Contributor

If the display name or description are going to be visible to users then we can't take this change. Loc cut-off was over a week ago.

@davkean

This comment has been minimized.

Member

davkean commented Nov 8, 2018

@tmeschter Too late for what release?

@tmeschter

This comment has been minimized.

Contributor

tmeschter commented Nov 8, 2018

@davkean 15.9 RTW.

@davkean

This comment has been minimized.

Member

davkean commented Nov 8, 2018

Oh, completely missed the branch here. @jainaashish Is this deliberate for 15.9? We'll need approval for the change itself and loc changes. To avoid approval for the loc changes, we can turn off visible, and then turn on visible in master.

//cc @Pilchie

@jainaashish

This comment has been minimized.

Contributor

jainaashish commented Nov 8, 2018

@Pilchie already mentioned about these loc changes and the fact that they will only be localized in dev16. We'll mention that once we ask for the approval for 15.9.1

@dasMulli

This comment has been minimized.

dasMulli commented Nov 8, 2018

But even having it in there with Visible="false" would fix the VS restore?

@davkean

This comment has been minimized.

Member

davkean commented Nov 8, 2018

Yes.

@davkean

This comment has been minimized.

Member

davkean commented Dec 10, 2018

@jainaashish What's going on with this PR?

@jainaashish

This comment has been minimized.

Contributor

jainaashish commented Dec 10, 2018

Last I knew, we had to bring it for 15.9.x approval but afterwards I don't know what happened. @rrelyea what happened to this?

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