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.targets per package #417

Closed
ctaggart opened this issue Dec 4, 2014 · 58 comments
Closed

paket.targets per package #417

ctaggart opened this issue Dec 4, 2014 · 58 comments

Comments

@ctaggart
Copy link
Contributor

ctaggart commented Dec 4, 2014

When I adopted Paket, I was expecting paket.targets files per NuGet package to soon happen. It was mentioned before in #197. I'd really like to see this happen. Something like this:

<Import Project="..\..\..\packages\FSharp.Core\paket.targets" Condition="..\..\..\packages\FSharp.Core\paket.targets" />
<Import Project="..\..\..\packages\Newtonsoft.Json\paket.targets" Condition="..\..\..\packages\Newtonsoft.Json\paket.targets" />

There are way to many changes being made to the project files and this causes lots of noise in the diff and some frustration when merging. An example is this pull request I just made for FAKE: fsprojects/FAKE#602

@forki
Copy link
Member

forki commented Dec 4, 2014

I know that this is a requested feature and I already started working on it. But it's not an easy one.

Regarding the FAKE changes. That's not that bad. The real issue was that I didn't make that Paket 0.17 transition before, so that you had both, the improved Paket output and your FSharp.Core changes in one Commit. But it's still good progress:

image

@forki
Copy link
Member

forki commented Dec 9, 2014

sneak peek for v0.19:

image

@isaacabraham
Copy link
Contributor

awesome

@ctaggart
Copy link
Contributor Author

ctaggart commented Dec 9, 2014

That is so beautiful!

You may wish to stick with lowercase paket.targets instead of Paket.targets since all the other files are lowercase. Since they are auto-generated, it doesn't matter much to me. May be that actually helps differentiate it from the other paket.targets used for msbuild targets.

@panesofglass
Copy link
Contributor

Lovely! Does this mean packages need to publish paket.targets files?

@ctaggart
Copy link
Contributor Author

ctaggart commented Dec 9, 2014

No, they get generated by Paket when it unzips the nupkg.

@forki
Copy link
Member

forki commented Dec 10, 2014

how about generated.paket.targets this will make it easy to find?

@forki
Copy link
Member

forki commented Dec 10, 2014

There is still one issue left:

We can't create the targets files per package since we have to create different When clauses if the csproj file already contains the same framework assemblies.

So we have two options:

  1. Create a targets file per package and project and put it under packages
    • How can we create a good naming pattern to match projects and targets files?
  2. Create a targets file per project (which contains all packages) and put it next to the project.
    • If the project is call MyProject.csproj we could call these MyProject.paket.targets
    • One could gitignore all *.paket.targets

What do you guys think?

/cc @agross @mexx

@TWith2Sugars
Copy link
Contributor

I'd prefer option 1 with MyProject.paket.targets

@forki
Copy link
Member

forki commented Dec 10, 2014

that wouldn't work since in one repository can be man MyProject.csproj files in different folders.

@agross
Copy link
Contributor

agross commented Dec 10, 2014

If we go for option 1 we should regenerate the project structure and put the targets there.

@agross
Copy link
Contributor

agross commented Dec 10, 2014

packages/Foo
packages/targets/src/Bar/Bar.csproj.paket.targets
src/Bar/Bar.csproj
src/Bar/paket.references

@TWith2Sugars
Copy link
Contributor

how about to keep it all at the same level myproject.projectguid.paket.target?

@agross
Copy link
Contributor

agross commented Dec 10, 2014

I'm against using GUIDs, they make it hard to reason about the relationship between ??proj and targets file.

@TWith2Sugars
Copy link
Contributor

Not a new guid, the one that has been assigned to the *proj file.

@agross
Copy link
Contributor

agross commented Dec 10, 2014

I think my argument still holds true ;-)

IMHO, GUIDs are for machines, not for human consumption.

@TWith2Sugars
Copy link
Contributor

It's already in there and referenced by the .sln. This is just to make sure the target file is unique, if we keep the myproject on the front it should be easy to see which target file matches to the _proj. I don't think I've seen that many projects with the same name._proj.

@TWith2Sugars
Copy link
Contributor

Fair enough :)

@TWith2Sugars
Copy link
Contributor

Ah but aren't these target files mainly used for the machines ;)

@forki
Copy link
Member

forki commented Dec 10, 2014

I vote for 2) ;-)

@TWith2Sugars
Copy link
Contributor

@forki I'd be happy with either way, overall having .targets in regardless of where is still a huge benefit.

@forki
Copy link
Member

forki commented Dec 10, 2014

I mean this still looks like good progress:

image

@TWith2Sugars
Copy link
Contributor

nice

currently sitting here waiting for this to be merged to try it.

@forki
Copy link
Member

forki commented Dec 10, 2014

you might want to test latest 0.19-alpha ;-)

already dogfooding: 99c0381

@TWith2Sugars
Copy link
Contributor

@forki boom, works like a dream.

If everything is inside .paket.target then will you still need the true?

@forki
Copy link
Member

forki commented Dec 10, 2014

If everything is inside .paket.target then will you still need the true?

the what?

@forki
Copy link
Member

forki commented Dec 10, 2014

omg

there seems to be no way to get VS restore working - and even the nuget guys don't do that anymore.

@forki
Copy link
Member

forki commented Dec 10, 2014

Seems we have to discontinue automatic VS restore if we want target files.
NuGet team came to the same conclusion: http://docs.nuget.org/docs/reference/package-restore

Instead we (as a comunity) could work on @hmemcpy's https://github.com/hmemcpy/Paket.VisualStudio and add restore to it.

@TWith2Sugars
Copy link
Contributor

I personally never used pakets VS auto restore so it's something I wouldn't even notice if it went.

@forki
Copy link
Member

forki commented Dec 10, 2014

I know that at least @ovatsus and @vasily-kirichenko think this is essential.

@TWith2Sugars
Copy link
Contributor

@vasily-kirichenko
Copy link
Contributor

Yes, auto restore is essential. I cannot tell people that they must run something called Paket before they are able to build their solution.

@forki
Copy link
Member

forki commented Dec 10, 2014

@TWith2Sugars how would that help?

It's really really sad. Instead of supporting PreSolutionBuild event in VS (which is already available in proper MSBuild) they built a nuget addin and shipped it in the VS box. This sucks big time.

Question is if a paket VS addin would be an acceptable solution.

@ctaggart
Copy link
Contributor Author

To support the people who want to clone the project and build it from Visual Studio without touching the command line, a Paket Visual Studio extension would be an acceptable solution. This is similar to having the NUnit or xUnit extension in order to run unit tests from Visual Studio.

@maartenba
Copy link

@forki Nothing of interest in http://www.nuget.org/packages/NuGetEnablePackageRestore/ ? This used to be NuGet package restore long, long ago and worked from within VS.

@ovatsus
Copy link

ovatsus commented Dec 10, 2014

Forcing to install a Paket extension in VS in order for your project to build is even worse than running the paket command line.
Any open source .Net project should be able to just build by opening the sln. It's a real barrier to new users if they always have to go look at the readme to check what's the requirement for every specific project

@agross
Copy link
Contributor

agross commented Dec 10, 2014

@Uli-Armbruster also wouldn't be pleased to learn that automatic restore in VS was discontinued.

@forki
Copy link
Member

forki commented Dec 10, 2014

Ok I think there is no good solution to this. I propose the following:

  • we add the targets feature
  • people who want to use VS restore have to commit the targets file for now
  • people who trust their build.cmd can gitignore the file

This is far from perfect, but even for the autorestore people this might be a small benefit since they know they can nuke the targets file if a merge error occurs.

We will look for better solutions. Maybe we can talk to @jeffhandley and propose to open up the nuget addin.

What do you think?

@TWith2Sugars
Copy link
Contributor

Sounds like a reasonable workaround

@jeffhandley
Copy link

As was pointed out, these problems are precisely why we ditched msbuild-integrated restore. Within VS, our extension hooks into the SolutionBuild event and executes restore BEFORE VS launches msbuild. Outside VS, it requires that build scripts run nuget.exe restore. That achieved the results that we wanted: no-touch restore in VS, but very scriptable outside VS. It's worked out great.

@forki
Copy link
Member

forki commented Dec 10, 2014

@jeffhandley we know that. The only issue for paket is: we are not in the
Visual Studio box and I don't see a good chance to get in.
On Dec 10, 2014 9:25 PM, "Jeff Handley" notifications@github.com wrote:

As was pointed out, these problems are precisely why we ditched
msbuild-integrated restore. Within VS, our extension hooks into the
SolutionBuild event and executes restore BEFORE VS launches msbuild.
Outside VS, it requires that build scripts run nuget.exe restore. That
achieved the results that we wanted: no-touch restore in VS, but very
scriptable outside VS. It's worked out great.


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

@isaacabraham
Copy link
Contributor

@ovatsus why is an extension such a bad thing? there's a plug-in for nuget, there could (should?) be one for paket as well. In addition to doing the Nuget-esque pre-build steps, it could do other things as well such as integrated ability to modify paket via a GUI integrated into VS rather than having to fall back to the command line (or fsx).

@ovatsus
Copy link

ovatsus commented Dec 11, 2014

I'm not saying the extension is a bad thing, would be great to have the extension. What I'm saying is that I want all my projects to be able to just work by a simple git clone + open the .sln and build, without any special requirements.

@forki
Copy link
Member

forki commented Dec 11, 2014

Yes we all want this. But MS closed the door for us. Instead of fixing the
root cause they shipped an addin. Btw: this is not only bad for us it's
also a really bad solution for other IDEs. Simply calling MSBuild doesn't
work anymore - so every other IDE has to build a plug-in.

Now we have to find the best workaround.
On Dec 11, 2014 11:26 AM, "Gustavo Guerra" notifications@github.com
wrote:

I'm not saying the extension is a bad thing, would be great to have the
extension. What I'm saying is that I want all my projects to be able to
just work by a simple git clone + open the .sln and build, without any
special requirements.


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

@ovatsus
Copy link

ovatsus commented Dec 11, 2014

Well, right now, if you don't change anything, restore is working, so Paket even has that as added benefit on top of Nuget, it will work fine on other IDEs

@isaacabraham
Copy link
Contributor

@ovatsus but Nuget takes this approach via an addin - out of the box VS won't restore conventional NuGet packages either.

@forki
Copy link
Member

forki commented Dec 11, 2014

I might have found a (hacky) way. Stay tuned.

@isaacabraham
Copy link
Contributor

@forki
Copy link
Member

forki commented Jan 6, 2015

We need to check out http://stackoverflow.com/a/24055340/145701

@ctaggart
Copy link
Contributor Author

I'm closing this because the new MSBuild world moves dependencies to an external project.lock.json that should be .gitignored which solves this issue and also my biggest grievance. Adding support for project.lock.json is in #736.

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

No branches or pull requests

10 participants