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

Adds binding redirect for applicable assemblies only #1187

Merged
merged 5 commits into from Nov 4, 2015

Conversation

Projects
None yet
4 participants
@mrinaldi
Contributor

mrinaldi commented Nov 3, 2015

Fixes #809.

/cc @isaacabraham

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 3, 2015

Member

I think there is an issue with using things like /binding-redirect-adds-referenced-assemblies-only/before/Project1/Project1.fsproj

when I run paket install for the main Paket project then this will get affected.

we should rename fsproj files in the before folder to fsprojtemplate and remove the suffix when we copy the files to temp.

Member

forki commented Nov 3, 2015

I think there is an issue with using things like /binding-redirect-adds-referenced-assemblies-only/before/Project1/Project1.fsproj

when I run paket install for the main Paket project then this will get affected.

we should rename fsproj files in the before folder to fsprojtemplate and remove the suffix when we copy the files to temp.

@isaacabraham

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Nov 3, 2015

Contributor

In terms of behaviour, am I correct in thinking that rather than inserting BRs from all dependencies, it now checks what dependencies there are for each project one by one, and generates specific BRs for each of them?

Contributor

isaacabraham commented Nov 3, 2015

In terms of behaviour, am I correct in thinking that rather than inserting BRs from all dependencies, it now checks what dependencies there are for each project one by one, and generates specific BRs for each of them?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 3, 2015

Member

Just to clarify: my comment was only referring to the integration tests. We
need to protect integration test data against run of paket update from the
root of the repo.
On Nov 3, 2015 1:26 PM, "Isaac Abraham" notifications@github.com wrote:

In terms of behaviour, am I correct in thinking that rather than inserting
BRs from all dependencies, it now checks what dependencies there are for
each project one by one, and generates specific BRs for each of them?


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

Member

forki commented Nov 3, 2015

Just to clarify: my comment was only referring to the integration tests. We
need to protect integration test data against run of paket update from the
root of the repo.
On Nov 3, 2015 1:26 PM, "Isaac Abraham" notifications@github.com wrote:

In terms of behaviour, am I correct in thinking that rather than inserting
BRs from all dependencies, it now checks what dependencies there are for
each project one by one, and generates specific BRs for each of them?


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

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Nov 3, 2015

Contributor

oops 😊

done in 142e28d

Contributor

mrinaldi commented Nov 3, 2015

oops 😊

done in 142e28d

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Nov 3, 2015

Contributor

@isaacabraham kind of. It doesn't generate BRs for each referenced dependency, but for required referenced dependency only.

That means it'll only generate BRs for dependencies that are referenced and have a higher version than the version required for another assembly.

For example:
AutoFixture.Xunit 3.36.9 requires xunit.extensions ≥ 1.8.0.1549 && < 2.0.0, but references xunit.extensions 1.8.0.1549.

This PR will only generate BRs when needed. That means if a project has a dependency on AutoFixture.Xunit 3.36.9 but depends on xunit.extensions 1.9.2, it'll generate BR for xunit.extensions.

However, if the project references AutoFixture.Xunit 3.36.9 and xunit.extensions 1.8.0.1549, no BR will be generated.

Contributor

mrinaldi commented Nov 3, 2015

@isaacabraham kind of. It doesn't generate BRs for each referenced dependency, but for required referenced dependency only.

That means it'll only generate BRs for dependencies that are referenced and have a higher version than the version required for another assembly.

For example:
AutoFixture.Xunit 3.36.9 requires xunit.extensions ≥ 1.8.0.1549 && < 2.0.0, but references xunit.extensions 1.8.0.1549.

This PR will only generate BRs when needed. That means if a project has a dependency on AutoFixture.Xunit 3.36.9 but depends on xunit.extensions 1.9.2, it'll generate BR for xunit.extensions.

However, if the project references AutoFixture.Xunit 3.36.9 and xunit.extensions 1.8.0.1549, no BR will be generated.

@isaacabraham

This comment has been minimized.

Show comment
Hide comment
@isaacabraham

isaacabraham Nov 3, 2015

Contributor

@mrinaldi so that's even better than what I was thinking - basically the minimal set of BRs that are required. LGTM.

Contributor

isaacabraham commented Nov 3, 2015

@mrinaldi so that's even better than what I was thinking - basically the minimal set of BRs that are required. LGTM.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 3, 2015

Member

is there something we should write into the docs?

Member

forki commented Nov 3, 2015

is there something we should write into the docs?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki
Member

forki commented Nov 3, 2015

/cc @dnauck

@dnauck

This comment has been minimized.

Show comment
Hide comment
@dnauck

dnauck Nov 3, 2015

Contributor

What about transient dependencies that needs BRs?

E.g. Dependency 1 has transient dependency on Newtonsoft.Json 5.x and Dependency 2 has transient dependency on Newtonsoft.Json 6.x ?

Looks like your PR (just read the text here) will not generate a BR for Newtonsoft.Json in this case but this is required.

Contributor

dnauck commented Nov 3, 2015

What about transient dependencies that needs BRs?

E.g. Dependency 1 has transient dependency on Newtonsoft.Json 5.x and Dependency 2 has transient dependency on Newtonsoft.Json 6.x ?

Looks like your PR (just read the text here) will not generate a BR for Newtonsoft.Json in this case but this is required.

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Nov 3, 2015

Contributor

@dnauck actually, it does.

Using this paket.dependencies

source https://www.nuget.org/api/v2/

nuget PushoverQ.Json
nuget Newtonsoft.Json.Schema

And this paket.references

PushoverQ.Json
Newtonsoft.Json.Schema

This is the resulting app.config:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
<runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
  <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>
  <dependentAssembly>
    <assemblyIdentity name="Microsoft.ServiceBus" publicKeyToken="31bf3856ad364e35" culture="neutral" />
    <bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="3.0.0.0" />
  </dependentAssembly>
</assemblyBinding></runtime></configuration>
Contributor

mrinaldi commented Nov 3, 2015

@dnauck actually, it does.

Using this paket.dependencies

source https://www.nuget.org/api/v2/

nuget PushoverQ.Json
nuget Newtonsoft.Json.Schema

And this paket.references

PushoverQ.Json
Newtonsoft.Json.Schema

This is the resulting app.config:

<?xml version="1.0" encoding="utf-8"?>
<configuration>
<runtime><assemblyBinding xmlns="urn:schemas-microsoft-com:asm.v1">
  <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>
  <dependentAssembly>
    <assemblyIdentity name="Microsoft.ServiceBus" publicKeyToken="31bf3856ad364e35" culture="neutral" />
    <bindingRedirect oldVersion="0.0.0.0-999.999.999.999" newVersion="3.0.0.0" />
  </dependentAssembly>
</assemblyBinding></runtime></configuration>
@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Nov 4, 2015

Contributor

Added 7b3533e to make sure that even if --createnewbindingfiles is specified, the app.config is not created if no binding redirect is required.

Contributor

mrinaldi commented Nov 4, 2015

Added 7b3533e to make sure that even if --createnewbindingfiles is specified, the app.config is not created if no binding redirect is required.

@mrinaldi

This comment has been minimized.

Show comment
Hide comment
@mrinaldi

mrinaldi Nov 4, 2015

Contributor

@forki We can improve the docs describing the new behavior, albeit the old behavior is not described.
In my opinion, however, the current docs are ok.

Contributor

mrinaldi commented Nov 4, 2015

@forki We can improve the docs describing the new behavior, albeit the old behavior is not described.
In my opinion, however, the current docs are ok.

forki added a commit that referenced this pull request Nov 4, 2015

Merge pull request #1187 from mrinaldi/binding_redirect_referenced_only
Adds binding redirect for applicable assemblies only

@forki forki merged commit 659a827 into fsprojects:master Nov 4, 2015

2 checks passed

continuous-integration/appveyor AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Nov 4, 2015

Member

cool! thanks so much!

Member

forki commented Nov 4, 2015

cool! thanks so much!

@mrinaldi mrinaldi deleted the mrinaldi:binding_redirect_referenced_only branch Nov 4, 2015

@mrinaldi mrinaldi referenced this pull request Dec 2, 2015

Closed

.NET 4.6.1 support? #1270

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