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 removes references which are under user control #1746

Closed
Stift opened this Issue Jun 22, 2016 · 17 comments

Comments

Projects
None yet
2 participants
@Stift
Contributor

Stift commented Jun 22, 2016

Description

References which are under user control should not be removed by Paket. Paket removes always references and let our build fail

Repro steps

We have a silverlight class library project which we use for testing. We add e.g. System.Net to that library and want to use that in our code. When we add the nuget dependency Microsoft.Bcl.Async then System.Net is removed because the for net40 this is specified as reference.

Expected behavior

We should be able to have control over that reference. But Paket always removes references. Also specifying <Paket>False</Paket> does not help.

Actual behavior

We always lose the references after a paket run.

Known workarounds

none. Use paket 2 as this behaviour seems new in Paket 3.

Related information

N/A

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

Argh. Please create a repro sample and temporarily downgrade to 2.x

This is related to the fact that --hard is now the default. But it should
not remove manual references that are marked with paket = false.
On Jun 22, 2016 10:21 AM, "Christian Fiebrig" notifications@github.com
wrote:

Description

References which are under user control should not be removed by Paket.
Paket removes always references and let our build fail
Repro steps

We have a silverlight class library project which we use for testing. We
add e.g. System.Net to that library and want to use that in our code. When
we add the nuget dependency Microsoft.Bcl.Async then System.Net is
removed because the for net40 this is specified as reference.
Expected behavior

We should be able to have control over that reference. But Paket always
removes references. Also specifying False does not help.
Actual behavior

We always lose the references after a paket run.
Known workarounds

none. Use paket 2 as this behaviour seems new in Paket 3.
Related information

N/A


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1746, or mute the thread
https://github.com/notifications/unsubscribe/AADgNIMlhjIinSWPb7GkyxVWpWu9h_fLks5qOPCQgaJpZM4I7hM-
.

Member

forki commented Jun 22, 2016

Argh. Please create a repro sample and temporarily downgrade to 2.x

This is related to the fact that --hard is now the default. But it should
not remove manual references that are marked with paket = false.
On Jun 22, 2016 10:21 AM, "Christian Fiebrig" notifications@github.com
wrote:

Description

References which are under user control should not be removed by Paket.
Paket removes always references and let our build fail
Repro steps

We have a silverlight class library project which we use for testing. We
add e.g. System.Net to that library and want to use that in our code. When
we add the nuget dependency Microsoft.Bcl.Async then System.Net is
removed because the for net40 this is specified as reference.
Expected behavior

We should be able to have control over that reference. But Paket always
removes references. Also specifying False does not help.
Actual behavior

We always lose the references after a paket run.
Known workarounds

none. Use paket 2 as this behaviour seems new in Paket 3.
Related information

N/A


You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
#1746, or mute the thread
https://github.com/notifications/unsubscribe/AADgNIMlhjIinSWPb7GkyxVWpWu9h_fLks5qOPCQgaJpZM4I7hM-
.

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

Ok. Took me a while to have a minimal repro to understand what happens.

So, if you take the repro here , then first is fine when you do .paket\paket install. But when you set <Private>True</Private> on the reference System.Net in the SilverlightClassLibrary.csproj and run .paket\paket install again, then it removes the dependency.
The check should be softer. In the reference-tag the include-attribute it could also contain a specifc version or sometimes also a subnode with a hint path. All these should be ignored by paket and let as-is because the user controls these.

Contributor

Stift commented Jun 22, 2016

Ok. Took me a while to have a minimal repro to understand what happens.

So, if you take the repro here , then first is fine when you do .paket\paket install. But when you set <Private>True</Private> on the reference System.Net in the SilverlightClassLibrary.csproj and run .paket\paket install again, then it removes the dependency.
The check should be softer. In the reference-tag the include-attribute it could also contain a specifc version or sometimes also a subnode with a hint path. All these should be ignored by paket and let as-is because the user controls these.

forki added a commit that referenced this issue Jun 22, 2016

forki added a commit that referenced this issue Jun 22, 2016

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

is a0d66a7 the change you meant? It's still keeping things green

Member

forki commented Jun 22, 2016

is a0d66a7 the change you meant? It's still keeping things green

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

yes. Exactly that change makes it fail.

Contributor

Stift commented Jun 22, 2016

yes. Exactly that change makes it fail.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

according to the integration test it keeps it. can you please check again?

Member

forki commented Jun 22, 2016

according to the integration test it keeps it. can you please check again?

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

you need to add the dependency microsoft.bcl.async v1.0.168

Contributor

Stift commented Jun 22, 2016

you need to add the dependency microsoft.bcl.async v1.0.168

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

this is the one which references system.net for net40

Contributor

Stift commented Jun 22, 2016

this is the one which references system.net for net40

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

ok. I can reproduce. It moves it down to it's own Choose nodes (which is expected).

That said. If I change it to

<Reference Include="System.Net" >
  <Private>True</Private>
  <Paket>False</Paket>
</Reference>

I'd expect it to stay. Unfortunately Paket still thinks it can take over control. I consider this a bug

Member

forki commented Jun 22, 2016

ok. I can reproduce. It moves it down to it's own Choose nodes (which is expected).

That said. If I change it to

<Reference Include="System.Net" >
  <Private>True</Private>
  <Paket>False</Paket>
</Reference>

I'd expect it to stay. Unfortunately Paket still thinks it can take over control. I consider this a bug

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

The problem that I see in that case, that System.Net is only defined for net40. Do you think in this case it should take the control away? It takes a lot of knowledge and investigation to find this out. For general developers it's really hard.

Contributor

Stift commented Jun 22, 2016

The problem that I see in that case, that System.Net is only defined for net40. Do you think in this case it should take the control away? It takes a lot of knowledge and investigation to find this out. For general developers it's really hard.

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

it's what the package specifies.
Anyways. a fix is underway that allows you to add paket = false

Member

forki commented Jun 22, 2016

it's what the package specifies.
Anyways. a fix is underway that allows you to add paket = false

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

I agree with that, but still it feels like paket steals this!

Contributor

Stift commented Jun 22, 2016

I agree with that, but still it feels like paket steals this!

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

I really see it as: we fix it according to the package specification. But
we allow you to know better.
On Jun 22, 2016 1:05 PM, "Christian Fiebrig" notifications@github.com
wrote:

I agree with that, but still it feels like paket steals this!

https://camo.githubusercontent.com/223ca839418af93739a63949f550ee069cd29448/687474703a2f2f692e67697068792e636f6d2f5653694e66736c38564952496b2e676966


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1746 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNN8gci3Nlwk-5lp_wzUD4J70GATQks5qORcEgaJpZM4I7hM-
.

Member

forki commented Jun 22, 2016

I really see it as: we fix it according to the package specification. But
we allow you to know better.
On Jun 22, 2016 1:05 PM, "Christian Fiebrig" notifications@github.com
wrote:

I agree with that, but still it feels like paket steals this!

https://camo.githubusercontent.com/223ca839418af93739a63949f550ee069cd29448/687474703a2f2f692e67697068792e636f6d2f5653694e66736c38564952496b2e676966


You are receiving this because you commented.
Reply to this email directly, view it on GitHub
#1746 (comment),
or mute the thread
https://github.com/notifications/unsubscribe/AADgNN8gci3Nlwk-5lp_wzUD4J70GATQks5qORcEgaJpZM4I7hM-
.

forki added a commit that referenced this issue Jun 22, 2016

@forki forki closed this in aa63e44 Jun 22, 2016

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

I understand this. At least the fix gives me a way to work with. I don't have a better solution for now. The only thing that I can think of, is also to define my other dependencies.

BTW, thx for the immediate reaction.

Contributor

Stift commented Jun 22, 2016

I understand this. At least the fix gives me a way to work with. I don't have a better solution for now. The only thing that I can think of, is also to define my other dependencies.

BTW, thx for the immediate reaction.

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

I tested it and it seems to work, but a <Paket>False</Paket> is not needed. Is this is intended by you?

Contributor

Stift commented Jun 22, 2016

I tested it and it seems to work, but a <Paket>False</Paket> is not needed. Is this is intended by you?

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

yeah. there was another bug(?) in exactly the same line. If you add "private" then it thought it was not a framework reference. But your sample shows that this assumption was wrong

Member

forki commented Jun 22, 2016

yeah. there was another bug(?) in exactly the same line. If you add "private" then it thought it was not a framework reference. But your sample shows that this assumption was wrong

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Jun 22, 2016

Member

anyways marking it with paket = false is a safe bet

Member

forki commented Jun 22, 2016

anyways marking it with paket = false is a safe bet

@Stift

This comment has been minimized.

Show comment
Hide comment
@Stift

Stift Jun 22, 2016

Contributor

ok. thx.

Contributor

Stift commented Jun 22, 2016

ok. thx.

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