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

Project package settings aren't persisted when specified in source #5312

Closed
srivatsn opened this issue May 31, 2017 · 14 comments
Closed

Project package settings aren't persisted when specified in source #5312

srivatsn opened this issue May 31, 2017 · 14 comments

Comments

@srivatsn
Copy link

From @terrajobst on April 22, 2017 2:16

I've helped a customer fix an issue after they migrated their project.json project to the new .csproj system.

Actual Behavior

The result looks like this:

  • The project has an old-school AssemblyInfo.cs with assembly level attributes.
  • The project is configured to not generate the assembly info file
  • When showing the package property pages, it shows these values in the UI
  • Changing the value and saving neither persists them in the AssemblyInfo.cs nor the .csproj
  • Building a NuGet package results in having a .nupkg that use the default values of the MSBuild properties, not matching the UI which displays the values from AssemblyInfo.cs

Expected Behavior

There seems to be two locations for the configuration of certain properties: MSBuild or hand-written AssemblyInfo.cs. Since the property pages are primarily for the NuGet package, they should match the values that are passed to the packaging task.

This can either be achieved by reading the produced assembly manifest and initialize the properties from there or by not using the values from AssemblyInfo.cs at all.

Copied from original issue: dotnet/project-system#2051

@srivatsn
Copy link
Author

We are supposed to persist changes to the assembly attributes if generateassemblyinfo is false.

@srivatsn
Copy link
Author

From @terrajobst on May 4, 2017 18:16

We are supposed to persist changes to the assembly attributes if generateassemblyinfo is false.

Sure, but that fix by itself doesn't address the issue that these values aren't used by pack to produce the NuGet package, right?

@srivatsn
Copy link
Author

From @davkean on May 4, 2017 23:32

@terrajobst Agreed.

@srivatsn
Copy link
Author

I confirmed that changing values in the package prop page updates assemblyinfo.cs - However as @terrajobst points out pack doesn't use those values. I forget how this was supposed to work, when this was originally discussed at dotnet/sdk#2. I'll move the discussion to NuGet.

@srivatsn
Copy link
Author

@rohit21agrawal is pack supposed to work using the info in assemblyinfo.cs?

@rohit21agrawal
Copy link
Contributor

no, we only use msbuild variables and targets to collect any values.

@terrajobst
Copy link

@srivatsn, wouldn't the right fix be to raise the values from the assemblyinfo.cs file when (1) the generation is suppressed and (2) the file exists? It seems to me that we don't want all build participants having to know that either mode is possible.

@srivatsn
Copy link
Author

That sounds reasonable but it feels a little bizarre to be parsing cs files in a task. @davkean @nguerrera do you remember if this came up in the original discussion?

@terrajobst
Copy link

That sounds reasonable but it feels a little bizarre to be parsing cs files in a task.

Agreed, but that's what VS already does to populate the dialog, no?

@srivatsn
Copy link
Author

No in VS, the project system talks to the language service to get that information and not in a task.

@nguerrera
Copy link

Parsing the file in a task is supremely impractical. We'd have to support 3 languages and either create a DLL hell situation depending on compiler API or risk getting it wrong. We can look in to raising from compiled assembly via FileVersionInfo. Turns out we need this code for generating satellite with assembly generation turned off.

@terrajobst
Copy link

terrajobst commented Jun 16, 2017

We can look in to raising from compiled assembly via FileVersionInfo.

I like that.

However, if it's impossible I'd argue we should stop supporting parsing AssemblyInfo.cs in the new project system and only use MSBuild properties. The higher order for me as a customer is that my NuGet package/assembly info has symmetry.

@mishra14
Copy link
Contributor

Closing as this is not a NuGet task.

@davkean
Copy link

davkean commented Nov 29, 2017

@mishra14 Help me understand your conclusion here.

@mishra14 mishra14 added this to the 4.6 milestone Nov 30, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

6 participants