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- read target framework from packages.config file - #760

Merged
merged 3 commits into from
Apr 9, 2015

Conversation

theimowski
Copy link
Member

references #609 #759

Not sure if this is enough, couldn't find anywhere info re the targetFramework attribute possible values.
Tested on a simple packages.config with net451 requirement

{ Name = PackageName packageName
VersionRequirement = VersionRequirement(VersionRange.Exactly version, PreReleaseStatus.No)
Sources = sources
ResolverStrategy = ResolverStrategy.Max
Settings = InstallSettings.Default
Settings = { InstallSettings.Default with FrameworkRestrictions = restrictions } //Requirements.parseRestrictions
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

what's about this comment?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

shouldn't be there, fixing

@forki
Copy link
Member

forki commented Apr 9, 2015

@Thorium, @mausch interested in beta-testing this?

@Thorium
Copy link
Member

Thorium commented Apr 9, 2015

I'll test...

@Thorium
Copy link
Member

Thorium commented Apr 9, 2015

If you have multiple projects having package.config files, some of them use .Net 4.5, some Portable, some Silverlight, etc.
And you use technologies like Castle-IoC or Rx, that are used in many frameworks.
Now it just parses one of the frameworks and forgets other ones.
nuget Castle.Core 3.3.0 framework: sl50
This is wrong, I would say this is not ready yet.
But what should actually happen in multi-case?
a) remove the target totally
b) somehow include list of different targets
c) add multiple lines to paket.dependencies and paket.lock

@forki
Copy link
Member

forki commented Apr 9, 2015

I'm voting for a)

@theimowski
Copy link
Member Author

It forgets the others because it takes only the one with highest version of the package - there's a yellow warning when you have multiple versions for same package

@Thorium
Copy link
Member

Thorium commented Apr 9, 2015

I have same versions, but still:
c:\MyApp\UI\package.config:

    <package id="Rx-Core" version="2.2.5" targetFramework="sl50" />

c:\MyApp\Logics\package.config:

    <package id="Rx-Core" version="2.2.5" targetFramework="net451" />

result of c:\MyApp\paket.dependencies is:

    nuget Rx-Core 2.2.5 framework: sl50

@theimowski
Copy link
Member Author

Ok so Then in case of multiple different targetframework should we omit the framework restriction?

@Thorium
Copy link
Member

Thorium commented Apr 9, 2015

I also vote for a)
as it is simple and clear solution.

@theimowski
Copy link
Member Author

ok so here it comes, rebased on master - @Thorium is this acceptable now?

forki added a commit that referenced this pull request Apr 9, 2015
convert from nuget- read target framework from packages.config file -
@forki forki merged commit 5f61865 into fsprojects:master Apr 9, 2015
@forki
Copy link
Member

forki commented Apr 9, 2015

LGTM

@Thorium
Copy link
Member

Thorium commented Apr 9, 2015

Thanks Tomasz!

@theimowski
Copy link
Member Author

You're welcome! ;)

@Thorium
Copy link
Member

Thorium commented Apr 10, 2015

Ok, one more problem by this conflicts of nuget targetframework and the real TargetFrameworkVersion.

If I make a new C#-class library and add reference to for example NServiceBus.Log4Net, by-default Visual Studio will make packages.config have:

    <package id="NServiceBus.Log4Net" version="1.0.0" targetFramework="net45" userInstalled="true" />

Then if people update the .NET-version of their projects, by re-targetting to .NET 4.5.1, Visual Studio will modify the .csproj-file to have <TargetFrameworkVersion>v4.5.1</TargetFrameworkVersion> but the packages.config is not updated. The project still builds and works ok.

But now, if I convert to Paket, it seems to be ok, but the build will fail. And the reason is that Paket will add to .csproj-file only a condition:

    <When Condition="$(TargetFrameworkIdentifier) == '.NETFramework' And $(TargetFrameworkVersion) == 'v4.5'">

...which won't hit, because of the conflicting 4.5 vs 4.5.1.

How could these mismatches between csproj TargetFrameworkVersion and nuget targetFramework detected?

@forki
Copy link
Member

forki commented Apr 10, 2015

how is the paket.dependencies file for this case?
The idea is that if we have targetframework 4.5 then Paket should install >= 4.5
So the bug might be elsewhere

@Thorium
Copy link
Member

Thorium commented Apr 10, 2015

Generated paket.dependencies (Paket 0.40.0):

    source https://www.nuget.org/api/v2
    source https://www.myget.org/F/particular

    nuget log4net 2.0.0 framework: net45
    nuget NServiceBus 5.0.0 framework: net45
    nuget NServiceBus.Log4Net 1.0.0 framework: net45

and paket.lock:

    NUGET
      remote: https://www.nuget.org/api/v2
      specs:
        log4net (2.0.0) - framework: net45
        NServiceBus (5.0.0) - framework: net45
        NServiceBus.Log4Net (1.0.0) - framework: net45
          NServiceBus (>= 5.0.0 < 6.0.0)
          log4net (>= 2.0.0 < 3.0.0)

@forki
Copy link
Member

forki commented Apr 10, 2015

we should generate >= net45 as described in http://fsprojects.github.io/Paket/nuget-dependencies.html#Framework-restrictions

care to send a PR?

@theimowski
Copy link
Member Author

ah ok that's my bad, I can send a fix, but @Thorium go on if you'd like to

@Thorium
Copy link
Member

Thorium commented Apr 10, 2015

I'll do...

@Thorium
Copy link
Member

Thorium commented Apr 10, 2015

Actually, if we have restriction ">= net45" and ">= net451" then the later one containing the another should be enough. So minor version conflicts could be just merged. But "net45" and "net40-client" would be better to not be merged.

@forki
Copy link
Member

forki commented Apr 10, 2015

we already have a function which can do this. see https://github.com/fsprojects/Paket/blob/master/tests/Paket.Tests/DependencySetSpecs.fs#L28

@Thorium
Copy link
Member

Thorium commented Apr 10, 2015

Great, I was thinking of some kind of difficult folding, but I don't have to do it then!
Added PR #768.

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.

None yet

3 participants