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

convert-from-nuget should create .targets for importing nuget binaries #197

Closed
pchalamet opened this issue Sep 29, 2014 · 25 comments
Closed

Comments

@pchalamet
Copy link

convert-from-nuget (as of 0.4.9.0) converts nuget binary references to a bunch of <When>/<Reference>.
This is pretty bad imho:

  • changing dependencies implies modifying project file
  • there is no reason to store such information inside the csproj as this can be generated from scratch using package definition when installing/restoring packages

Well, something like that in a project referencing Rx would then be modified as follow:

<Import Project="$(SolutionDir)\.paket\paket.targets" />
<Import Project="$(SolutionDir)\packages\Rx-Core\Paket.targets" />
<Import Project="$(SolutionDir)\packages\Rx-Interfaces\Paket.targets" />
<Import Project="$(SolutionDir)\packages\Rx-Linq\Paket.targets" />

and a generated file for Rx-Core could be (sitting inside packages\pkg-name\Paket.targets):

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.0'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)\packages\Rx-Core\lib\Net40\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)\packages\Rx-Core\lib\Net45\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)\packages\Rx-Core\lib\Net45\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == 'Silverlight' And $(SilverlightVersion) == 'v5.0'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)\packages\Rx-Core\lib\SL5\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
    <When Condition="$(TargetFrameworkIdentifier) == 'Silverlight'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)\packages\Rx-Core\lib\SL5\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>
  </Choose>
</Project>
@pchalamet pchalamet changed the title convert-from-nuget should create .targets for importing nugget binaries convert-from-nuget should create .targets for importing nuget binaries Sep 29, 2014
@agross
Copy link
Contributor

agross commented Sep 29, 2014

A little formatting helped me understand what you were getting at! Sounds like a good idea to me! 👍

@forki
Copy link
Member

forki commented Sep 29, 2014

👍 I second that.

I think it's a really great idea and it gives real advantages:

  • shorter proj file. Actually it's trimmed to base class references + source files. That's great.
  • since the targets files are in gitignored folder, every paket update will have at least one diff and that's in the lockfile. exactly where it belongs. and that's even if the libs folder in the package changes. That's awesome!

@forki
Copy link
Member

forki commented Sep 29, 2014

rocket

@rojepp
Copy link
Contributor

rojepp commented Sep 29, 2014

This change does sound nice, but it has me worried. I know I've had to modify .*proj files manually, but I can't think of a single example right now.
How does this work for JS libs for web sites?

@forki
Copy link
Member

forki commented Sep 29, 2014

we still would not touch any manual installations.

It's not in this sample, but paket adds marker sub-nodes to indicate "I was installed by paket". If Paket finds nodes to a dll without the marker we skip it.

How does this work for JS libs for web sites?

Very good point. I'd like to hear others about that.

(related: there are people who think it's not a good idea to use nuget for js libs - #199. Please don't discuss the general issue here. Just interested in where to put content installations)

@pchalamet
Copy link
Author

It is possible to use a Condition attribute on the import. This would be possible to discover Paket's imports then:

<Import Project="..." Condition="$(Paket) != ''" />

@forki
Copy link
Member

forki commented Sep 30, 2014

If we use paket.targets as file names then this should be enough

@dedale
Copy link
Contributor

dedale commented Sep 30, 2014

Good idea Pierre, I told something similar to Steffen a couple of days ago, but it's way better to open an issue with samples :-)

@ctaggart
Copy link
Contributor

ctaggart commented Oct 2, 2014

I really like this idea of using paket.targets files!

@forki
Copy link
Member

forki commented Oct 2, 2014

@ilkerde what do you think about this?

@forki
Copy link
Member

forki commented Oct 6, 2014

there are still open questions:

During installation we would add <Import Project="..."/> to the proj file.
The referenced targets file will not be commited (right?).
If we clone a repo and the packages folder is empty the targets file is missing. I assume VS will not load the project. That would be a showstopper.

If we update the targets file (new version installed or something like that) we create no change in the proj file. VS will not reload.

@ctaggart
Copy link
Contributor

ctaggart commented Oct 6, 2014

Just add a Condition for if the paket.targets file Exists. NuGet does this:

<Import Project="$(SolutionDir)\.nuget\NuGet.targets" Condition="Exists('$(SolutionDir)\.nuget\NuGet.targets')" />

This will allow Visual Studio to load the project if the file is missing.

@forki
Copy link
Member

forki commented Oct 6, 2014

sounds good.

@ilkerde
Copy link
Contributor

ilkerde commented Oct 12, 2014

@forki I'm really thinking hard about it. I also try to do this by hand, but it doesn't seem to work. I have VS2013 and a simple sln/csproj, tried to externalize a package reference as @pchalamet describes, but VS doesn't seem to recognize the ref at all. Any hints on how to get a working sample?

@forki
Copy link
Member

forki commented Oct 12, 2014

No sorry. I didn't look into this very deeply.
On Oct 12, 2014 11:47 AM, "ilkerde" notifications@github.com wrote:

@forki https://github.com/forki I'm really thinking hard about it. I
also try to do this by hand, but it doesn't seem to work. I have VS2013 and
a simple sln/csproj, tried to externalize a package reference as
@pchalamet https://github.com/pchalamet describes, but VS doesn't seem
to recognize the ref at all. Any hints on how to get a working sample?


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

@ilkerde
Copy link
Contributor

ilkerde commented Oct 12, 2014

I'll try to get a working sample, but all my efforts so far (VS2013 + VS2012 with simple sln/csproj referencing our beloved Newtonsoft.Json) failed. @pchalamet Maybe you can provide us with a working sample?

@pchalamet
Copy link
Author

I have a hackish patch sitting there: https://github.com/pchalamet/Paket/tree/targets
Got this for one of my project and it does compile and is correctly seen by VS13.
Note that Imports must be under <Project>.

for a csproj referencing Rx:

<Project>
....
  <Import Project="$(SolutionDir)\.paket\paket.targets" />
  <Import Project="$(SolutionDir)/packages/Rx-Core/Paket.targets" />
  <Import Project="$(SolutionDir)/packages/Rx-Interfaces/Paket.targets" />
  <Import Project="$(SolutionDir)/packages/Rx-Linq/Paket.targets" />
  <Import Project="$(SolutionDir)/packages/Rx-PlatformServices/Paket.targets" />
</Project>

And Rx-Core's Paket.targets:

<Project xmlns="http://schemas.microsoft.com/developer/msbuild/2003">
  <Choose>
    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.0'">
      <ItemGroup>
        <Reference Include="System.Reactive.Core">
          <HintPath>$(SolutionDir)/packages\Rx-Core\lib\net40\System.Reactive.Core.dll</HintPath>
          <Private>True</Private>
        </Reference>
      </ItemGroup>
    </When>

@pchalamet
Copy link
Author

Btw, I failed to make it work for Moq package (work for other packages) - that is why I've not attempt a PR (and some clean up has to be done anyway). My test project is https://github.com/pchalamet/cassandra-sharp (have to keep only net45 solution and add MinimumVSver otherwise Paket badly breaks).

@agross
Copy link
Contributor

agross commented Oct 12, 2014

Can you please elaborate on the "breaking badly" part?

(insert animated gif of Walter White here)

Alex

Alexander Groß
Tiny phone, tiny mail

On Sun, Oct 12, 2014 at 2:45 PM, Pierre Chalamet notifications@github.com
wrote:

Btw, I failed to make it work for Moq package (work for other packages) - that is why I've not attempt a PR (and some clean up has to be done anyway). My test project is https://github.com/pchalamet/cassandra-sharp (have to keep only net45 solution and add MinimumVSver otherwise Paket badly breaks).

Reply to this email directly or view it on GitHub:
#197 (comment)

@pchalamet
Copy link
Author

SolutionFile::AddPaketFolder() requires MinimumVisualStudioVersion.

@theimowski
Copy link
Member

MinimumVisualStudioVersion should have been fixed in #221.

@forki
Copy link
Member

forki commented Oct 13, 2014

Close this?
On Oct 13, 2014 12:06 PM, "Tomasz Heimowski" notifications@github.com
wrote:

MinimumVisualStudioVersion should have been fixed in #221
#221.


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

@pchalamet
Copy link
Author

Probably. I've not rebased since few days. I will try to rebuild the patch for *.targets.

@pchalamet
Copy link
Author

I won't try to patch Paket anymore as I have implemented my own package manager; I'm now able to reference packages as targets files. Much better indeed.

@ctaggart
Copy link
Contributor

ctaggart commented Nov 3, 2014

:( I've really been looking forward to this feature. It is one that I've marked as required for us to upgrade from 0.8.5. It has several benefits that will increase adoption and reduce friction.

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

No branches or pull requests

8 participants