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

Binding redirects not being updated on paket install #1218

Closed
mavnn opened this issue Nov 13, 2015 · 18 comments · Fixed by #1235
Closed

Binding redirects not being updated on paket install #1218

mavnn opened this issue Nov 13, 2015 · 18 comments · Fixed by #1235

Comments

@mavnn
Copy link
Contributor

mavnn commented Nov 13, 2015

I'm having some problems isolating the exact scenarios causing this, but the issue appears to have appeared somewhere between 2.20 and 2.23.

I'll carry on having a look :)

@forki
Copy link
Member

forki commented Nov 13, 2015

/cc @mrinaldi

@mrinaldi
Copy link
Contributor

@mavnn If you remove the binding redirects and run the paket install, does it generate the BRs you're expecting?

2.22 has introduced a new behavior where paket only generates BRs for applicable assemblies.
However, paket never deletes the BR. It either generates new ones or replace the old ones, if needed.

That seems to be the case. It seems that a BR generated by 2.20 exists in App.config and after running 2.23 it just won't replace it because it's never been needed actually.

I've been wondering if we should mark the BRs generated by paket - with a <!--Paket--> comment, for instance - and remove them if they're not needed anymore. That shouldn't "fix" this scenario though, since older releases don't mark the BRs.

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

I think what's happening is that the old bindings exist, but then because they "aren't needed" they don't get updated when you re-run install after a change - leaving a binding redirect to the (now) wrong version.

@mrinaldi
Copy link
Contributor

@mavnn and @forki What do you suggest?

Should we let the user delete it manually - leaving it as is

Should we mark the BRs paket generates and remove them, if they're not needed anymore - fixing this behavior for the new versions, but leaving as is for the old ones

Should we warn the user while paket install that a BR is not needed anymore and should be deleted by hand

Any other ideas?

@forki
Copy link
Member

forki commented Nov 13, 2015

/cc @dnauck @isaacabraham

I don't have strong opions here. What do others think?

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

I have ~800 projects to go through, so manual doesn't really apply here :D

I can script it for myself, but would a --hardRedirects or similar option make sense? What's more of a concern is that this is a bit of a ticking time bomb for people next time they do a paket update of anything with unnecessary redirects. Any ideas for getting around that?

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

Especially nasty because the fact the redirects haven't changed doesn't normally show up until you get an assembly load fail at runtime.

@mrinaldi
Copy link
Contributor

I like the CLI option.
Even better would be to have both the CLI and paket.dependencies redirects option for that.

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

What do people think - should the default behaviour be that paket assumes it owns binding redirects?

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

@forki @isaacabraham @dnauck

@mavnn
Copy link
Contributor Author

mavnn commented Nov 13, 2015

Just for reference...

#r "System.Xml"
#r "System.Xml.Linq"

let xml = """<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <runtime>

  <assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
  <dependentAssembly>
    <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>
    <assemblyIdentity name="Newtonsoft.Json" publicKeyToken="30ad4fe6b2a6aeed" culture="neutral" />
    <bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="7.0.0.0" />
  </dependentAssembly>
</assemblyBinding></runtime>
  <startup>
    <supportedRuntime version="v4.0" sku=".NETFramework,Version=v4.5" />
  </startup>
</configuration>"""

open System.Xml
open System.Xml.Linq

let doc = XDocument.Parse xml

doc.Descendants(XName.Get("assemblyBinding", "urn:schemas-microsoft-com:asm.v1"))
|> Seq.toList
|> List.iter (fun el -> el.Remove())
|> printfn "%A"

@isaacabraham
Copy link
Contributor

Perfect behaviour would be for Paket to manage Paket-generated BRs and not amend others. This means deleting existing Paket generated ones that are obsolete etc.

Problems could occur if you manually added a BR for the same assembly I suppose.

@forki
Copy link
Member

forki commented Nov 16, 2015

I think we can use the already existing --hard parameter for clean up. Also adding "paket" flags to the generated bits would be great

@isaacabraham
Copy link
Contributor

One thing I have noticed - if you move from the "old" BR format (without ) to the new one, the old BRs will remain. Worse still, new versions won't appear until you manually remove the old ones. Might it be worth putting some node or attribute at e.g. the assemblyBinding level to indicate that Paket BRs exist inside this?

@mrinaldi
Copy link
Contributor

if you move from the "old" BR format (without ) to the new one, the old BRs will remain

That's expected, unless you specify the --hard, which should remove all the old BRs.

Worse still, new versions won't appear until you manually remove the old ones.

Can you provide a repro?

@isaacabraham
Copy link
Contributor

I'll try create a clean solution to prove it, but you should be able to repro by creating a paket dependency to an older version of a dll which needs a BR, manually removing the node, and then calling paket update.

@mrinaldi
Copy link
Contributor

Sorry man, I still can't reproduce.
I'll have to wait for your repro case.

@forki
Copy link
Member

forki commented Mar 8, 2016

is his still relevant?

@forki forki closed this as completed Apr 6, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants