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

Add exclude projects with templates flag for pack #1540

Conversation

pms1969
Copy link
Contributor

@pms1969 pms1969 commented Mar 23, 2016

Needed a way to exclude projects that have templates from the packing process. This brings the output in line with how nuget behaves, and prevents problems for package consumers where they have the same dll available in multiple packages.

@forki
Copy link
Member

forki commented Mar 23, 2016

I don't really understand that

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

can't explain the travis problem. looks like a recursive issue somewhere.

To explain the need for the flag: when I package a project A that has a dependency on project B, without this flag, I see project B's dll in the lib output even though it is also added as a dependency. So when I next take a dependency on package A, I also get package B as a transient dependency, which also contains package B's dll. That's sub-optimal at best, and leads to confusion, and may lead to mistakes (someone linking directly to project B's dll in project A's package folder. It's also not in line with how nuget packages up projects.

IIRC, this was the way I originally coded this stuff around one of my symbols PR's, but that broke some peoples expectations of output from pack, and as a result, you changed that bit of code back to maintain compatibility (rightly so). This is just to enable that feature again for those that need it (namely me, but I can see it being used by just about everyone else, since this is the way nuget behaves).

Hope that make sense.

@forki
Copy link
Member

forki commented Mar 23, 2016

A that has a dependency on project B, without this flag, I see project
B's dll in the lib output even though it is also added as a dependency.

That's the real issue right?
On Mar 23, 2016 10:25, "Paul Saunders" notifications@github.com wrote:

can't explain the travis problem. looks like a recursive issue somewhere.

To explain the need for the flag: when I package a project A that has a
dependency on project B, without this flag, I see project B's dll in the
lib output even though it is also added as a dependency. So when I next
take a dependency on package A, I also get package B as a transient
dependency, which also contains package B's dll. That's sub-optimal at
best, and leads to confusion, and may lead to mistakes (someone linking
directly to project B's dll in project A's package folder. It's also not in
line with how nuget packages up projects.

IIRC, this was the way I originally coded this stuff around one of my
symbols PR's, but that broke some peoples expectations of output from pack,
and as a result, you changed that bit of code back to maintain
compatibility (rightly so). This is just to enable that feature again for
those that need it (namely me, but I can see it being used by just about
everyone else, since this is the way nuget behaves).

Hope that make sense.


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

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

yes. but that's how paket has worked for some time, and there was some kick back on the behavior change when I changed it last time.

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

just found where I changed it before.... #1417
Still looking for the issue that was raised.

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

and this is the issue that was raised off the back of it.

#1429

@forki
Copy link
Member

forki commented Mar 23, 2016

I thought earier we were talking about adding deps or not.

I'm not completely sure what we did, but did we say that we want both the B.dll + the B dependency in the same A package? That sounds weird.

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

it is weird. Unfortunately, I can't shoot you a package at the moment as I'm at work, but I can assure you that that is the way it currently works.

@forki
Copy link
Member

forki commented Mar 23, 2016

yeah I believe you, but did we discuss this earlier?
I thought we discussed how we decide to add dependency or put the dll in. But did someone rely on the fact that we do both?

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

yes, someone was relying on the fact that we do both.

We had discussed this, and agreed the behavior in this fix is the desired behavior, but I think that may have been lost by the time the issue was raised. (#1429)

You might want to consider just removing the fag in v3 and making the default behavior not to include (tho I'm sure you'll get equal kick back).

@kevinbosman
Copy link
Contributor

There's inconsistency in the way the packaging works, depending on where you launch paket pack from. I've been trying to formulate a bug report around this, but not sure if it relates to this issue or not.

Given a scenario with 2 projects A and B, each with a paket.template, A referencing B:
a) If you pack from the solution (root) folder, then A.nupkg will contain a reference to B, and correctly not contain B.dll
b) If you pack from project A folder (root/A), then you will get B.dll included in A.nupkg (with or without include-referenced-projects

@forki
Copy link
Member

forki commented Mar 23, 2016

/cc @kolektiv @inosik

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

@kevinbosman, I can indeed confirm that packaging from root does not include the dll (i.e. without specifying a template name). I've pretty much always specified the template name, so ran into this problem.

I'm now not quite sure what way forward is. Clearly the behavior when specifying a template is incorrect, but is being relied on.

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

Just done some investigation, and I can't change the behavior of our build scripts because we need to exclude some templates under certain conditions. Ultimately, the 2 ways of packing should be consistent.

@forki
Copy link
Member

forki commented Mar 23, 2016

Ultimately, the 2 ways of packing should be consistent

yes that template specifc version sounds like a bug to me. we should not add package + dll

@forki
Copy link
Member

forki commented Mar 23, 2016

@pms1969 can you change this PR to make it not pack B.dll when it adds B as dependency?

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

Perhaps I should just supply a new PR. It's a small change.

@pms1969
Copy link
Contributor Author

pms1969 commented Mar 23, 2016

submitted as #1543

@forki forki closed this Mar 23, 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 this pull request may close these issues.

3 participants