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

Paket 3 unconditionally removes all binding redirects from Web.config #1665

Closed
konste opened this issue May 5, 2016 · 24 comments
Closed

Paket 3 unconditionally removes all binding redirects from Web.config #1665

konste opened this issue May 5, 2016 · 24 comments
Labels

Comments

@konste
Copy link
Contributor

konste commented May 5, 2016

WebConfigRepro.zip

Small Web project attached as a repro. Paket Install command removes all binding redirects from Web.config file.

I have tried with "redirects: off" and "redirects: on" at the top of paket.dependencies - no change in behavior.

It also worth nothing to mention that it still forcibly adds a space before XML tag closing.

And one more - Paket changes encoding of Web.config file by inserting BOM symbol in the beginning.

All those three things are better be left alone, at least when "redirects: off" explicitly specified.

@forki
Copy link
Member

forki commented May 17, 2016

@mrinaldi @isaacabraham can you please comment on this one? thanks

@forki
Copy link
Member

forki commented May 18, 2016

I think this could be a result of me removing the --hard parameter.
Maybe we still need it for this

@forki forki added the bug label May 18, 2016
@forki
Copy link
Member

forki commented May 18, 2016

I think I will bring this back as new parameter --clean-redirects (with default no). What do you think?

@konste
Copy link
Contributor Author

konste commented May 18, 2016

Two points:

  1. when paket.dependencies says "redirects: off" it means do not touch config files. It does not need any other command line parameter to explain or clarify or enforce its intention.
  2. Unless you mark each binding redirect with the comment "added by Paket" (similarly to how you mark assembly references injected by Paket to project files) there is no way to distinguish binding redirects which are safe to remove from those which are not.

@forki
Copy link
Member

forki commented May 18, 2016

"redirects: off" is not really related this

@konste
Copy link
Contributor Author

konste commented May 18, 2016

Another consideration: since .NET 4.5.1 VS can generate binding redirects pretty much automatically with <AutoGenerateBindingRedirects>true</AutoGenerateBindingRedirects> in the project file.

@forki
Copy link
Member

forki commented May 18, 2016

We can't assume VS exists.

@konste
Copy link
Contributor Author

konste commented May 18, 2016

True, but in this case actually msbuild is doing it and we rely on it anyway.

@forki
Copy link
Member

forki commented May 18, 2016

let me readd the parameter - this changes the default back to paket 2 mode. I think this will work for you.

@konste
Copy link
Contributor Author

konste commented May 18, 2016

Anyway my point is - there are two possible kinds of the problems:

  1. Paket edits config files in some unexpected way or
  2. Paket does not touch config files when it should

Between those two #2 is much better and safer. If not sure - do not touch it rather!

@konste
Copy link
Contributor Author

konste commented May 18, 2016

--clean-redirects set to Yes would clean ALL redirects again - those added by Paket and those which were not. There is no way to separate them.
What scenario you consider which would warrant existence of --clean-redirects: Yes?

@konste
Copy link
Contributor Author

konste commented May 18, 2016

I understand that simply --clean-redirects:No would just work for us, but now I'm thinking about potential users confusion about that new option. I doubt it is needed.

@forki
Copy link
Member

forki commented May 18, 2016

it is needed ;-)
your workflow is only one of many.

@forki
Copy link
Member

forki commented May 18, 2016

--clean-redirects will remove all redirects that are not set by paket. this is a command you only run once and after that all your redirects are completely under paket's control.

@konste
Copy link
Contributor Author

konste commented May 18, 2016

Meaningful action is not removing all binding redirects altogether. Meaningful action is to try to set binding redirects correctly, which means some may need to be added, some may need to be removed and some may need to be adjusted. That's the functionality I would expect from "redirects: on". When it is set to off I expect Paket to simply bypass opening of any config files.

@konste
Copy link
Contributor Author

konste commented May 18, 2016

-- --clean-redirects will remove all redirects that are not set by paket.

So are they marked somehow?

@konste
Copy link
Contributor Author

konste commented May 18, 2016

Anyway, it is time for me to stop burning midnight oil and hit the bed. I trust you to implement whatever solution you deem correct as long as there is an option to leave config files alone :-)

@isaacabraham
Copy link
Contributor

Whoa. Just read the last part of this. @forki are we saying that the new defaults in v3 will remain or has been reverted back to v2?

@forki
Copy link
Member

forki commented May 18, 2016

It's reverted to v2 behavior. This means manual redirects are only touched
if you specify that parameter.
On May 18, 2016 12:19 PM, "Isaac Abraham" [email protected] wrote:

Whoa. Just read the last part of this. @forki https://github.com/forki
are we saying that the new defaults in v3 will remain or has been reverted
back to v2?


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1665 (comment)

@isaacabraham
Copy link
Contributor

So hard not by default?

@forki
Copy link
Member

forki commented May 18, 2016

yes. not for this case, but for everything else where hard was used. TBH i think it was unfortunate to use the same parameter for this and the other use case in the first place.

@smoothdeveloper
Copy link
Contributor

I'm not following the minute details of the issue, but it seems important to preserve existing bindings, unless those are related to paket managed assembly dependencies, in which case paket should add/update those.

What I'm reading is that @konste had custom bindings that are not related to paket dependencies and that he likes to get those preserved (disregarding paket.dependencies or having to call an extra command) and @forki that made a convenience --clean-redirects option that will explicitly clean the redirects if user wanted to do that.

I'm not seeing how that will address @konste issue though, he'd expect paket to not touch any redirect (he has many entered manually) besides those related to AWSSDK (and transitive dependencies) since it is the only reference he has in his project.

@forki
Copy link
Member

forki commented May 18, 2016

Yes we don't touch automatically anymore.
On May 18, 2016 14:11, "Gauthier Segay" [email protected] wrote:

I'm not following the minute details of the issue, but it seems important
to preserve existing bindings, unless those are related to paket managed
assembly dependencies, in which case paket should add/update those.

What I'm reading is that @konste https://github.com/konste had custom
bindings that are not related to paket dependencies and that he likes to
get those preserved (disregarding paket.dependencies or having to call an
extra command) and @forki https://github.com/forki that made a
convenience --clean-redirects option that will explicitly clean the
redirects if user wanted to do that.

I'm not seeing how that will address @konste https://github.com/konste
issue though, he'd expect paket to not touch any redirect (he has many
entered manually) besides those related to AWSSDK (and transitive
dependencies) since it is the only reference he has in his project.


You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub
#1665 (comment)

@smoothdeveloper
Copy link
Contributor

@forki gotcha thanks! 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

4 participants