aspnet / RoslynCodeDomProvider

Roslyn CodeDOM provider
MIT License
84 stars 43 forks source link

XDT creates inconsistent line endings #137

Open StephenMolloy opened 2 years ago

StephenMolloy commented 2 years ago

Seems that in some circumstances, the XDT transforms used upon package install/uninstall can leave inconsistent line endings in the config file which results in a dialog being popped up by VS which isn't a great experience. Especially since that dialog doesn't give much insight into why the line endings are inconsistent. (The scenario is when there are multi-line comments in the config file btw.)

When we write to the config file with our install.ps1 script, it re-normalizes all the line endings so there isn't any warning upon install. But when uninstalling (upgrading too?) our uninstall.ps1 runs first and the XDT after, so we can't inadvertently fix the issue.

One approach could be to abandon our XDT's entirely and do that work in our .ps1 scripts. That might be more fragile though, and I'm not sure this crops up often enough or meets the annoying-enough threshold for that risk.

The other thing to consider here is that neither XDT transforms nor un/install.ps1 scripts are supported in packageReference projects, so I'm not inclined to spend lots of resources to fix a scenario that is increasingly not recommended. (I don't know that there is a good strategy for handling these projects though. Running a build task to transform config if it does not see providers registered already? That doesn't help discoverability for anyone who wants to customize any settings. And it doesn't help us with a path to migrate previous customizations forward if any options change between versions. It's really a sad story that we have to put this burden on the developer when then install our package in packageReference projects... but I don't know that I want to make it even more confusing by half-handedly doing some default things sometimes in a non-obvious behind-the-scenes sort of way.)

CZEMacLeod commented 2 years ago

I'm not sure about uninstall/cleanup - but I would be much happier with a build task making the changes to web.config/app.config based on the current package version, and if required then adding some documented properties that can be included in the project file. The install.ps1 and/or xdt approach are not very reliable, and are already 'fragile'. Making it work for PackageReference and legacy packages.config using build tasks would seem to me to be a much more sensible approach. I would suggest replacing all the powershell script functions with MSBuild Tasks and just add them to the existing .targets file.

StephenMolloy commented 2 years ago

We obviously use build tasks to copy the roslyn binaries into the output. But web/app.config updates are a little different. If registering the providers with default options was the only concern, then a build task would be the obvious way to go. I think this was part of the idea behind packageReference getting rid of install.ps1 and XDT transforms - that nuget packages should only be distributing libraries and content. But they did not consider packages that distribute updateable content.

The scenario here partly falls into that updatable category. People want to customize the options these providers are configured with. The fragile things we do in install.ps1 to account for customization will also be fragile in an MSBuild task, except they will now be fragile on every build and not immediately visible/fixable in the IDE.

I'm not absolutely opposed to that approach, and have considered it before. But it's not at all straightforward or any more reliable. And quite frankly, I don't feel 100% comfortable with how much time I can put into it or how many resources we have on the team for testing all the various scenarios to make sure we're not screwing anybody up. I go back and forth on whether it's worth it. If the convenience of not having to manually update your config file once is worth the work and potential reliability/diagnosability risk.

Although, I actually filed this issue to have a public note of why folks might be seeing VS dialogs about fixing line endings. The install.ps1 vs .targets debate is tangentially lurking in the shadows. :smiling_imp: