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

Nuget package update reformats config files poorly or creates unwanted config files #7180

Closed
StingyJack opened this issue Aug 7, 2018 · 22 comments
Assignees
Labels
Functionality:Install The install command in VS/nuget.exe help wanted Considered good issues for community contributions. Priority:2 Issues for the current backlog. Style:Packages.Config Type:Bug

Comments

@StingyJack
Copy link
Contributor

The VS Feedback item has the original unredacted attachment illustrating the problem.

image
In this redacted example, you can see its removed the line breaks as well as changed the spacing before the closing element. Its changing the formatting of config files in this manner for projects that are not having any binding redirects added or changed, and for projects that aren't being targeted for the package update.

Its also creating new app.config files for projects that didnt have them and don't need them (class libraries).

All of these are a tedious venture to cleanup before checkin or submitting for code review. On larger solutions (> 50 projects) this can easily take an hour or more to clean up - every time a single package is updated in a project in the solution.

@mishra14 mishra14 added Type:Bug Area:Settings NuGet.Config and related issues labels Aug 10, 2018
@StingyJack
Copy link
Contributor Author

The general trashing of formatting is the result of using XmlDocument or XDocument. Other than parsing the file as text I dont think there is a way to preserve all existing formatting in a config file. I would consider that a defect in either those two classes, or in the decision to use XML files for .net configuration. FWIW, the ability to add comments to XML makes it better than JSON for this purpose, but I would expect this to be sensible about how much needless pending change churn it creates.

The creating of app.config files for projects that didnt have them and dont need them is a bug in nuget, as is the zero-change modification to an existing config file that causes pending changes in team explorer. Adding the binding redirect checkbox on the project property helps some, but is still kind of a bandaid.

To get around this as fast as possible, currently I can search for app.config files in Solution Explorer and delete all the ones with a pending add, then use Undo Unchanged from this extension (where did tfpt uu go?) to cleanup the csproj changes.

@StingyJack
Copy link
Contributor Author

@mishra14 - I'm sure you guys know how to do this stuff already, but if it helps address this issue faster, I figured out how to edit a part of the config file without wrecking the formatting of the rest of the file. Its not complete as far as adding/editing/removing specific assembly bindings (that code I think you would already have), but it does show how to avoid the unnecessary churn created from XmlDocument or XDocument.

This is an example where I had it rewrite the assembly bindings section only. Note the element closures and line breaks in the rest of the file are preserved.
image

@mishra14
Copy link
Contributor

mishra14 commented Sep 6, 2018

@PatoBeltran is currently working on changing the code that reads xml files in NuGet. He might be better able to address this issue.

cc: @jainaashish @nkolev92

@StingyJack
Copy link
Contributor Author

StingyJack commented Oct 31, 2018

Related issues...

#1527
#6018
#6465
#5385

@StingyJack
Copy link
Contributor Author

@mishra14 @PatoBeltran - any info about this?

@nkolev92
Copy link
Member

I don't think his changes affect the web.config, but rather the NuGet.config files only.
So I don't think anything has changed.

@StingyJack
Copy link
Contributor Author

@nkolev92 - is the "Settings" tag appropriate for this as its not settings related?

@nkolev92 nkolev92 added Style:Packages.Config Functionality:Install The install command in VS/nuget.exe and removed Area:Settings NuGet.Config and related issues labels Dec 26, 2018
@StingyJack
Copy link
Contributor Author

@nkolev92 - Also, this issue has had many reports on developer community and contrary to Alisa Gu's response, it is affecting all users of Visual Studio.

Its not a Packages.Config specific thing either, as installation of packages by Package ref also will create/modify app and web.config files for binding redirects.

@nkolev92
Copy link
Member

nkolev92 commented Dec 26, 2018

@StingyJack

NuGet itself does not modify the app/web config as part of the PackageReference restore/install (which is really restore in the background) workflow.

So if there are changes, it's a different component.

@StingyJack
Copy link
Contributor Author

StingyJack commented Dec 27, 2018

@nkolev92 - what is it that adds binding redirects to config files after/during package installation? What is it that adds app.config files to projects that didnt have/need them? I'm pretty sure I've seen that code and it is in the nuget client repo.

EDIT: If you are telling me that package ref will install packages but not create binding redirects when necessary, then that is a different problem. I cant have software that compiles but has runtime issues due to missing redirects. That's going to be yet another entry in the list of 1000 cuts..

@nkolev92
Copy link
Member

nkolev92 commented Dec 27, 2018

I'm pretty sure I've seen that code and it is in the nuget client repo.

You are correct, nuget does do that, but only in the case of packages.config. See for example that code is in a VisualStudio, while NuGet PackageReference also works the same in msbuild and dotnet.exe.
In PackageReference it works a bit differently, it's a different component that handles the binding redirects.

See https://docs.microsoft.com/en-us/dotnet/framework/configure-apps/how-to-enable-and-disable-automatic-binding-redirection.

If you are telling me that package ref will install packages but not create binding redirects when necessary

I'm just saying it's not NuGet specifically that does that anymore, rather a different component. Which is why I tagged it as "packages.config" only.

@StingyJack
Copy link
Contributor Author

@nkolev92 - Tagging this as packages.config means we will have to continue to deal with this burden because while the nuget team "supports" packages.config there will not be any attempts to fix bugs that do not overlap package ref.

A tool is only effective if it allows you to achieve more with a similar effort.

  • This defect causes us to have to do more work to manage something.
  • Package ref causes us to do even more work to compensate for all lost the features and functionality (and the conversion of projects). The general use case gains no benefit with using this over package.config.

Not a great set of choices you all have decided to leave us with. Throwing an unexpected pile of work in front of someone who was using a tool you made is a foul thing to do.

@StingyJack
Copy link
Contributor Author

@heng-liu and @zkat - just so its understood, this is still a problem for any developer who uses config files and nuget packages. I've just stopped complaining here since I've given a technique for how to fix the hard part and am now just waiting for you all to actually do something

@miaw7
Copy link

miaw7 commented Jul 3, 2020

@heng-liu @zkat i have the same problem and it really annoys me. Every time before checkin i need to check each config file and undo changes to all config files that were unexpectedly changed. It also adds configs for library projects. If you have many projects in one solution it is really a nightmare. It takes extra efforts and time. It would be great if we can get some kind of feedback

@heng-liu heng-liu self-assigned this Dec 7, 2020
@heng-liu heng-liu added this to the Sprint 180 - 2020.11.30 milestone Dec 7, 2020
@heng-liu
Copy link
Contributor

I tried the XDocument APIs, but seems none of them could keep the indent in one element(between attributes).
e.g. for the following xml file:

<compiler language="c#;cs;csharp" extension=".cs"
        type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.CSharpCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=2.0.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"
        warningLevel="4" compilerOptions="/langversion:default /nowarn:1659;1699;1701"/>
<compiler language="vb;vbs;visualbasic;vbscript" extension=".vb"
        type="Microsoft.CodeDom.Providers.DotNetCompilerPlatform.VBCodeProvider, Microsoft.CodeDom.Providers.DotNetCompilerPlatform, Version=2.0.1.0, Culture=neutral, PublicKeyToken=31bf3856ad364e35"
        warningLevel="4" compilerOptions="/langversion:default /nowarn:41008 /define:_MYTYPE=\&quot;Web\&quot; /optionInfer+"/>

If we save the above file in path1, and load the file and save it to path2

XDocument doc = XDocument.Load(path1, LoadOptions.PreserveWhitespace);
doc.Save(path2, SaveOptions.DisableFormatting);

Then the indent in path1 disappeared when loaded and saved in path2.

So looks like we need to implement a fundamental function to keep the whitespaces in one element(between attributes).
BTW, it affects legacy (non-SDK style) projects.

@heng-liu heng-liu added the help wanted Considered good issues for community contributions. label Jan 11, 2021
@heng-liu heng-liu removed their assignment Jan 11, 2021
@StingyJack
Copy link
Contributor Author

@heng-liu - I made a working POC a while ago

@heng-liu
Copy link
Contributor

Thanks @StingyJack !
Do you mind turning it into a PR so that people could review it?
Here is the CONTRIBUTING.md document if you would like to contribute. Thanks.

@StingyJack
Copy link
Contributor Author

Its a POC so I didnt bother with some of the niceties or adhering to any one particular style. I made it available to just about anyone to use however they wanted.

But I didn't create this incredible time vampire of a defect described in the OP thats just been getting ignored by its creator. It seemed to me that it was not getting fixed because the creator did not know how to go about fixing it. For that I am willing to help unblock them, but I should not be asked have to take on the entire burden of fixing it, including the inevitable PR grief and likely denial of the same.

Since you can add labels, I'm assuming you work for MSFT. I do not, so I'm not sure I'm even equipped to do this work since it may be inside VS.

@aortiz-msft aortiz-msft removed this from the Sprint 180 - 2020.11.30 milestone Jan 14, 2021
@martinrrm martinrrm self-assigned this Mar 4, 2022
@martinrrm martinrrm added this to the Sprint 2022-03 milestone Mar 7, 2022
@nkolev92 nkolev92 modified the milestones: Sprint 2022-04, Sprint 2022-05 Apr 25, 2022
@zivkan zivkan self-assigned this May 11, 2022
@zivkan
Copy link
Member

zivkan commented Jun 9, 2022

Hi @StingyJack

Regarding the creation of app.config files for class libraries, feel free to create a new issue for that feature request so we can track it separately, as the majority of this issue is discussing whitespace changes.

@zivkan
Copy link
Member

zivkan commented Jun 9, 2022

Team triage meeting: Regarding keeping whitespace in app.config and web.config files, after careful consideration, we have decided not to work on this issue due to high cost of implementation and the expectation that customers will continue to migrate to .NET (6+) and SDK style projects.

@StingyJack
Copy link
Contributor Author

feature request so we can track it separately, as the majority of this issue is discussing whitespace changes.

@zivkan - A customer reported a problem to you 4.5 years ago, you finally get around to triaging it, and your response is to tell the customer to open a new ticket?

And then leave an even older report open?

high cost of implementation

Wasnt this the hard part?

This specific issue burned at least 100 hours of my time over a 2-3 year period with something that should have been caught and flagged by QC/QA,
I dont know how this can be a triage activity 5 years after the first report was logged.
TBH I gave up on this (and many other problems the NuGet team created) actually being fixed a while ago, and it has nothing to do with the expectation you mentioned. I dont need or expect an answer or response to this but the NuGet team should at least have the decency to take these points as places where there is a real need for their improvement.

@jrmoreno1
Copy link

@zivkan : those of us still working on webform applications can't switch over to .net 7+ or SDK projects for everything (as SDK projects for webform seems to be off the menu dotnet/project-system#2670).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Functionality:Install The install command in VS/nuget.exe help wanted Considered good issues for community contributions. Priority:2 Issues for the current backlog. Style:Packages.Config Type:Bug
Projects
None yet
Development

No branches or pull requests

10 participants