Paket changes my config files #1248

Closed
haf opened this Issue Nov 22, 2015 · 49 comments

Comments

Projects
None yet
6 participants
@haf
Member

haf commented Nov 22, 2015

screen shot 2015-11-22 at 10 38 05

Do you know why?

XML should not contain BOMs imo, as no other language really support them.

@forki

This comment has been minimized.

Show comment
Hide comment
Member

forki commented Nov 23, 2015

@isaacabraham

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Nov 23, 2015

Contributor

Urgh. Don't know why that's there. I think this should be done as part of the cleanup of the formatting of the BRs anyway (you can see from the </assemblyBinding></runtime> line that it's not formatted properly).

Contributor

isaacabraham commented Nov 23, 2015

Urgh. Don't know why that's there. I think this should be done as part of the cleanup of the formatting of the BRs anyway (you can see from the </assemblyBinding></runtime> line that it's not formatted properly).

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 2, 2015

Contributor

How about Paket leave XML formatting alone? We have never ending problem with Paket always changing our config files (incl. those it should not be touching at all). It keeps removing spaces in lines like 'value="false" />' So it removes the space between doublequote and slash and does it for ALL configs. We would leave it like that, but some other automated tool has different idea what the right spacing is and it keep adding that space back! This creates enormous churn and makes reviewing config file changes very hard. Please, please let Paket NOT CHANGE existing XML formatting. Especially considering we have "redirects: off" at the top of paket.dependencies.

Contributor

konste commented Dec 2, 2015

How about Paket leave XML formatting alone? We have never ending problem with Paket always changing our config files (incl. those it should not be touching at all). It keeps removing spaces in lines like 'value="false" />' So it removes the space between doublequote and slash and does it for ALL configs. We would leave it like that, but some other automated tool has different idea what the right spacing is and it keep adding that space back! This creates enormous churn and makes reviewing config file changes very hard. Please, please let Paket NOT CHANGE existing XML formatting. Especially considering we have "redirects: off" at the top of paket.dependencies.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 2, 2015

Member

Yes it's definitely the plan to keep formatting as stable as possible. Will
try to add couple of tests tomorrow
On Dec 2, 2015 21:36, "Konstantin Erman" notifications@github.com wrote:

How about Paket leave XML formatting alone? We have never ending problem
with Paket always changing our config files (incl. those it should not be
touching at all). It keeps removing spaces in lines like 'value="false" />'
So it removes the space between doublequote and slash and does it for ALL
configs. We would leave it like that, but some other automated tool has
different idea what the right spacing is and it keep adding that space
back! This creates enormous churn and makes reviewing config file changes
very hard. Please, please let Paket NOT CHANGE existing XML formatting.


Reply to this email directly or view it on GitHub
#1248 (comment).

Member

forki commented Dec 2, 2015

Yes it's definitely the plan to keep formatting as stable as possible. Will
try to add couple of tests tomorrow
On Dec 2, 2015 21:36, "Konstantin Erman" notifications@github.com wrote:

How about Paket leave XML formatting alone? We have never ending problem
with Paket always changing our config files (incl. those it should not be
touching at all). It keeps removing spaces in lines like 'value="false" />'
So it removes the space between doublequote and slash and does it for ALL
configs. We would leave it like that, but some other automated tool has
different idea what the right spacing is and it keep adding that space
back! This creates enormous churn and makes reviewing config file changes
very hard. Please, please let Paket NOT CHANGE existing XML formatting.


Reply to this email directly or view it on GitHub
#1248 (comment).

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 2, 2015

Contributor

@forki, thank you for speedy response! It is always great to work with you!

Contributor

konste commented Dec 2, 2015

@forki, thank you for speedy response! It is always great to work with you!

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

@konste can you give me the app.config in question (which contains value = false) for a unit test?

Member

forki commented Dec 3, 2015

@konste can you give me the app.config in question (which contains value = false) for a unit test?

@konste

This comment has been minimized.

Show comment
Hide comment
Contributor

konste commented Dec 3, 2015

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

wow. even if we disable formatting it seems .NET always writes value="false"/> as value="false" />

Member

forki commented Dec 3, 2015

wow. even if we disable formatting it seems .NET always writes value="false"/> as value="false" />

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

I may look into it. Which exactly .NET Xml facility is employed here? XmlDocument? XDocument?

But why rewrite configs which are not touched a bit at all?

Contributor

konste commented Dec 3, 2015

I may look into it. Which exactly .NET Xml facility is employed here? XmlDocument? XDocument?

But why rewrite configs which are not touched a bit at all?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

that are two separate issues. one is we should not touched configs that we don't change logically. second is that we don't stuff outside of redirects

Member

forki commented Dec 3, 2015

that are two separate issues. one is we should not touched configs that we don't change logically. second is that we don't stuff outside of redirects

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

I understand that the second part is harder than it seems. Writing XML file is usually one call to a Save method (unless combination of reader and writer is used, which seems like overkill here), so we just need a way to preserve original formatting. So what .NET class is used to handle XML in Paket?

Contributor

konste commented Dec 3, 2015

I understand that the second part is harder than it seems. Writing XML file is usually one call to a Save method (unless combination of reader and writer is used, which seems like overkill here), so we just need a way to preserve original formatting. So what .NET class is used to handle XML in Paket?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

As far as I can tell from src\Paket.Core\BindingRedirects.fs XDocument is used. Then formatting preservation is as easy as calling Save like that: x.Save(writeFileName, SaveOptions.DisableFormatting);

Contributor

konste commented Dec 3, 2015

As far as I can tell from src\Paket.Core\BindingRedirects.fs XDocument is used. Then formatting preservation is as easy as calling Save like that: x.Save(writeFileName, SaveOptions.DisableFormatting);

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

^^ it's not I'm already doing this in my local version

Member

forki commented Dec 3, 2015

^^ it's not I'm already doing this in my local version

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

@konste with latest changes we should not touch files when we don't do logical changes. can you please test it?

Member

forki commented Dec 3, 2015

@konste with latest changes we should not touch files when we don't do logical changes. can you please test it?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

Is binary available? I don't have F# build environment at the moment.

Contributor

konste commented Dec 3, 2015

Is binary available? I don't have F# build environment at the moment.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

Yes it's in latest release.
On Dec 3, 2015 19:41, "Konstantin Erman" notifications@github.com wrote:

Is binary available? I don't have F# build environment at the moment.


Reply to this email directly or view it on GitHub
#1248 (comment).

Member

forki commented Dec 3, 2015

Yes it's in latest release.
On Dec 3, 2015 19:41, "Konstantin Erman" notifications@github.com wrote:

Is binary available? I don't have F# build environment at the moment.


Reply to this email directly or view it on GitHub
#1248 (comment).

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

Version 2.32.5 keeps updating all config files when I do "paket update" which otherwise does nothing.

Contributor

konste commented Dec 3, 2015

Version 2.32.5 keeps updating all config files when I do "paket update" which otherwise does nothing.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

what's the diff?

Member

forki commented Dec 3, 2015

what's the diff?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

Same - space before slash.

Contributor

konste commented Dec 3, 2015

Same - space before slash.

@forki

This comment has been minimized.

Show comment
Hide comment
Member

forki commented Dec 3, 2015

dog

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

nothing else? only the spaces?

Member

forki commented Dec 3, 2015

nothing else? only the spaces?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor
  1. One space before EACH />
  2. At the end of each config a line is added:
 <runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime>
  1. Fourth ".0" in versions is stripped again.
Contributor

konste commented Dec 3, 2015

  1. One space before EACH />
  2. At the end of each config a line is added:
 <runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime>
  1. Fourth ".0" in versions is stripped again.
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

ah ok. I assume 3 is the reason.

since I didn't find a way to tel the .NET API to keep formatting of that char I tried to come up with "logical changes comparison". If no logical changes => we don't touch the file at all.

But 3 indicates a logical change => we touch it. please just commit that removement of .0 and the try again.

BTW: sorry. but this stuff is really tricky (and annoying)

Member

forki commented Dec 3, 2015

ah ok. I assume 3 is the reason.

since I didn't find a way to tel the .NET API to keep formatting of that char I tried to come up with "logical changes comparison". If no logical changes => we don't touch the file at all.

But 3 indicates a logical change => we touch it. please just commit that removement of .0 and the try again.

BTW: sorry. but this stuff is really tricky (and annoying)

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

-- If no logical changes => we don't touch the file at all.

Can we make it: no changes in BindingRedirect => we don't touch the file at all?

Because in our case we have "redirects: off" and I would expect it to cut short all attempts to even read config files.

Contributor

konste commented Dec 3, 2015

-- If no logical changes => we don't touch the file at all.

Can we make it: no changes in BindingRedirect => we don't touch the file at all?

Because in our case we have "redirects: off" and I would expect it to cut short all attempts to even read config files.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

yeah that's the next step.

".0" in versions is stripped again.

why is that even in there if you have redirects: off?

Member

forki commented Dec 3, 2015

yeah that's the next step.

".0" in versions is stripped again.

why is that even in there if you have redirects: off?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

".0" in versions is stripped in paket.lock. Nothing to do with configs.

Contributor

konste commented Dec 3, 2015

".0" in versions is stripped in paket.lock. Nothing to do with configs.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

ok I'm confused now.

let's start over:

  • .0 is removed from lock file? That's intended and from other issue, right?
  • Nothing semantically changed in binding redirects file? Only reformatting?
Member

forki commented Dec 3, 2015

ok I'm confused now.

let's start over:

  • .0 is removed from lock file? That's intended and from other issue, right?
  • Nothing semantically changed in binding redirects file? Only reformatting?
@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

Both Right.

Contributor

konste commented Dec 3, 2015

Both Right.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

is there a chance for me to reproduce? Can you mail me and tell me what you did? I won't make it public.

Member

forki commented Dec 3, 2015

is there a chance for me to reproduce? Can you mail me and tell me what you did? I won't make it public.

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

I think the problem is that last line it adds - it is basically empty BindingRedirects section. So the logic seems to be - if we are not going to add BindingRedirects to config, but it does not even have corresponding section - empty section is added. And "while we are here" extra spaces are added too.

Contributor

konste commented Dec 3, 2015

I think the problem is that last line it adds - it is basically empty BindingRedirects section. So the logic seems to be - if we are not going to add BindingRedirects to config, but it does not even have corresponding section - empty section is added. And "while we are here" extra spaces are added too.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

ook just to confirm. can you commit that line and try again? does it stay stable and doesn't remove the space before the slashes?

Member

forki commented Dec 3, 2015

ook just to confirm. can you commit that line and try again? does it stay stable and doesn't remove the space before the slashes?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

Ok, here we go: when I replace this:

</configuration>

with this (literally!):

<runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime></configuration>

then Paket leaves that config alone and does not add extra spaces or change it in any other way.

But if I add this section formatted differently than all in one line, then it is ignored and config is reformatted. Weird.

Contributor

konste commented Dec 3, 2015

Ok, here we go: when I replace this:

</configuration>

with this (literally!):

<runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime></configuration>

then Paket leaves that config alone and does not add extra spaces or change it in any other way.

But if I add this section formatted differently than all in one line, then it is ignored and config is reformatted. Weird.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

GOOD!

that's what I wanted to hear - means we're on the right track. I will remove this tomorrow ;-)

Member

forki commented Dec 3, 2015

GOOD!

that's what I wanted to hear - means we're on the right track. I will remove this tomorrow ;-)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

And sorry for all the trouble...

Member

forki commented Dec 3, 2015

And sorry for all the trouble...

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 3, 2015

Contributor

No prob, thank you for the help! I wish I could contribute more than testing.

Contributor

konste commented Dec 3, 2015

No prob, thank you for the help! I wish I could contribute more than testing.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 3, 2015

Member

dude testing is most important since that guy is me:

wtf

Member

forki commented Dec 3, 2015

dude testing is most important since that guy is me:

wtf

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

@konste please retry agan. we now remove <runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime>

Member

forki commented Dec 4, 2015

@konste please retry agan. we now remove <runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1" /></runtime>

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 4, 2015

Contributor

2.32.7 is better and removing empty binding redirects section definitely works, but it is still not good enough. When config file does contain binding redirects section, then Paket still adds extra space! We have 17 files like that in solution. Still too much unnecessary churn.

Contributor

konste commented Dec 4, 2015

2.32.7 is better and removing empty binding redirects section definitely works, but it is still not good enough. When config file does contain binding redirects section, then Paket still adds extra space! We have 17 files like that in solution. Still too much unnecessary churn.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

don't really know how to fix that. yet
at least not with that .NET API

Member

forki commented Dec 4, 2015

don't really know how to fix that. yet
at least not with that .NET API

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

do you have bindind redirects completely disabled in paket? even for these 17 files?

Member

forki commented Dec 4, 2015

do you have bindind redirects completely disabled in paket? even for these 17 files?

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 4, 2015

Contributor

If that is what "redirects: off" at the top of paket.dependencies does - then yes.

Contributor

konste commented Dec 4, 2015

If that is what "redirects: off" at the top of paket.dependencies does - then yes.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 4, 2015

Member

@mrinaldi can we skip the redirects processing completely in this case?

Member

forki commented Dec 4, 2015

@mrinaldi can we skip the redirects processing completely in this case?

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Dec 4, 2015

Contributor

Yep, will do when I have the time.

Contributor

mrinaldi commented Dec 4, 2015

Yep, will do when I have the time.

@konste

This comment has been minimized.

Show comment
Hide comment
@konste

konste Dec 4, 2015

Contributor

For now I decided that space before slash is "standard enough" and I'm going to make all our configs look like that. Now Paket does not seem to change them anymore. One little concern though is that Paket still changes XML formatting as shown in the picture:
2015-12-04 12_03_20-start
But after I correct formatting manually Paket leaves it alone, which is good enough for all practical purposes. After all VS can reformat XML in one click.

Let's keep the issue open waiting for @mrinaldi to bypass config sniffing completely when possible.

Contributor

konste commented Dec 4, 2015

For now I decided that space before slash is "standard enough" and I'm going to make all our configs look like that. Now Paket does not seem to change them anymore. One little concern though is that Paket still changes XML formatting as shown in the picture:
2015-12-04 12_03_20-start
But after I correct formatting manually Paket leaves it alone, which is good enough for all practical purposes. After all VS can reformat XML in one click.

Let's keep the issue open waiting for @mrinaldi to bypass config sniffing completely when possible.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 9, 2015

Member

Latest version should not do this anymore

Member

forki commented Dec 9, 2015

Latest version should not do this anymore

@jackfoxy

This comment has been minimized.

Show comment
Hide comment
@jackfoxy

jackfoxy Dec 22, 2015

Yep, this is a problem for us too, add and update both are wiping out the binding redirects in our app.config files. (As of 24 hours ago using the latest from nuget. I see @forki 13 hours ago says the latest fixes this. If it changed within those 11 hours I have not tested.)

Yep, this is a problem for us too, add and update both are wiping out the binding redirects in our app.config files. (As of 24 hours ago using the latest from nuget. I see @forki 13 hours ago says the latest fixes this. If it changed within those 11 hours I have not tested.)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 23, 2015

Member

my comment was 13 days ago.
are you using redirect:force option?

Member

forki commented Dec 23, 2015

my comment was 13 days ago.
are you using redirect:force option?

@jackfoxy

This comment has been minimized.

Show comment
Hide comment
@jackfoxy

jackfoxy Dec 23, 2015

No, not using redirect or force. Here is the most common binding redirects Paket is overwriting. I can't remember why we need the FSharp.Core redirect, but we do need it. I think Paket wrote the <Paket>True</Paket> lines when I converted from nuget to paket. That may be the problem and why Paket thinks it can just eliminate these redirects.

<dependentAssembly>
<Paket>True</Paket>
<assemblyIdentity name="FSharp.Core" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="4.4.0.0" />
</dependentAssembly>
<dependentAssembly>
<Paket>True</Paket>
<assemblyIdentity name="Microsoft.WindowsAzure.Storage" publicKeyToken="31bf3856ad364e35" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="4.3.0.0" />
</dependentAssembly>

No, not using redirect or force. Here is the most common binding redirects Paket is overwriting. I can't remember why we need the FSharp.Core redirect, but we do need it. I think Paket wrote the <Paket>True</Paket> lines when I converted from nuget to paket. That may be the problem and why Paket thinks it can just eliminate these redirects.

<dependentAssembly>
<Paket>True</Paket>
<assemblyIdentity name="FSharp.Core" publicKeyToken="b03f5f7f11d50a3a" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="4.4.0.0" />
</dependentAssembly>
<dependentAssembly>
<Paket>True</Paket>
<assemblyIdentity name="Microsoft.WindowsAzure.Storage" publicKeyToken="31bf3856ad364e35" culture="neutral" />
<bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="4.3.0.0" />
</dependentAssembly>

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Dec 23, 2015

Member

Use "nuget fsharp.core redirects:force" in your dependencies file. We
needed to change that default.
On Dec 23, 2015 7:35 PM, "Jack Fox" notifications@github.com wrote:

No, not using redirect or force. Here is the most common binding redirects
Paket is overwriting. I can't remember why we need the FSharp.Core
redirect, but we do need it. I think Paket wrote the True
lines when I converted from nuget to paket. That may be the problem and why
Paket thinks it can just eliminate these redirects.

True True


Reply to this email directly or view it on GitHub
#1248 (comment).

Member

forki commented Dec 23, 2015

Use "nuget fsharp.core redirects:force" in your dependencies file. We
needed to change that default.
On Dec 23, 2015 7:35 PM, "Jack Fox" notifications@github.com wrote:

No, not using redirect or force. Here is the most common binding redirects
Paket is overwriting. I can't remember why we need the FSharp.Core
redirect, but we do need it. I think Paket wrote the True
lines when I converted from nuget to paket. That may be the problem and why
Paket thinks it can just eliminate these redirects.

True True


Reply to this email directly or view it on GitHub
#1248 (comment).

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