fsprojects / Paket

A dependency manager for .NET with support for NuGet packages and Git repositories.
https://fsprojects.github.io/Paket/
MIT License
1.99k stars 520 forks source link

Conditional ItemGroups on .NET version get in the way #1440

Open jackfoxy opened 8 years ago

jackfoxy commented 8 years ago

Not sure if this is entirely Paket's doing. There must be a use case for setting different ItemGroup .NET version conditionals for different versions of a package dll, but in my use case (and I would think this is the most common case) I am only every building whichever .NET version I specified for the project in Visual Studio. At best the conditionals just work and are clutter, but frequently Paket Update does not update them correctly. Most recently when I upgraded projects to .NET v4.6.1 Packet seemed to only know about v4.6 and set condtionals only as high as v4.6. (Yes, I tried setting net461 in the dependencies file.) Needless to say, nothing would build. It's also aggravating when there is only one version of a dll, and conditionals are built up for the only version that can build. (Hope this makes sense, taking a long weekend and might not be able to respond for a couple days.)

forki commented 8 years ago

Mhm we have the framework option that you can set on top of the dependencies file and that should work. Do you have a concrete repro case where it didn't?

forki commented 8 years ago

Installing all versions in conditionals is the default since this allows you to change your target framework whenever you like and also allows you to build for multiple target frameworks without reinstalling packages. When you set a target framework filter we install only a subset of the possible assemblies. This reduces clutter in the csproj files, but also makes target framework switching harder. The target framework filter even influences the package resolution.

Setting net461 as you described above should have worked to install the libs for you target framework. That's why I asked for a repro case.

jackfoxy commented 8 years ago

All of my problems over the last 2 months, and those of others on our team over many more months have resulted in significant manual rework of projects, so I cannot easily give you a test case. I just don't remember how to set things up. Instead I think I can make some recommendations below.

Installing all versions in conditionals is the default since this allows you to change your target framework whenever you like and also allows you to build for multiple target frameworks without reinstalling packages.

Something I have never wanted/needed to do, and none of the solutions we build at our company target multiple frameworks. This is useful for some solutions in the world, but I am sure the overwhelming majority do not need to do this.

I got off to a bad start with this issue report. Let me start over:

This is not just my experience, but I think I can speak for our whole team, which is F# focused, almost no C# whatsoever.

1) We are not interested in managing multiple .NET versions in our builds. Ever. If we ever care we can build it in.

2) There are only 3 use cases for Paket to ever modify a project file. This should be the default behavior of Paket.Update. If it cannot be the default behavior, there should be an easy to understand option to make it behave this way. a) A new reference, in which case insert the reference for the highest version of the package compatible with the .NET Framework version build for the project. b) A previous reference is no longer in the project's dependencies, in which case remove the reference from the project. c) The latest package has a higher release compatible with the .NET Framework version build for the project, in which case update the reference.

Comment -- we want to keep things simple, not solve every problem that can arise. It would still be useful to understand Paket's "opinion" about any other action it may think we need, but this should be written to the console or a message file, not updating the project by default.

3) Never update my binding redirects. Paket.Update frequently decides to delete redirects I have and add redirects I don't need. Once again, messages to the console or message file are good.

Final Comment -- Paket can be a beautiful tool, but up until now our experience with it has not followed the principle of least surprise. Default update behavior should be minimal updates to project files and no updates (but suggestions) to binding redirects.

forki commented 8 years ago

1) we already have the target framework filter which allows you to do this. I can think of a new mode "framework: autodetect" which would look through all project files and take the target framework that was specified (or throw if it is not unique)

2) this is exactly what should happen. That's why I wanted to know which additional things paket does that you don't want.

3) did you try to disable binding redirects from paket.dependencies file? That feature is indeed really really complex.

isaacabraham commented 8 years ago

AFAIK - binding redirects only kick in if you've explicitly turned them on (indeed, in the very early days it was the other way around). So just removing the command from the dependencies file will do the trick. In addition, from what I know (@mrinaldi can comment more here), Paket should no longer overwrite existing BRs for a specific assembly as all Paket-generated ones have a Paket node in them and only those will be modified.

mrinaldi commented 8 years ago

@isaacabraham is right. Disabling binding redirects from paket.dependencies should do the trick.

Paket will not remove existing BRs, unless --hard is used. However, Paket will add the Paket node to all existing BRs that it should generate and once it has the Paket node, it may be removed if it's not needed anymore.

Just to point out again though, nothing of this should happen if you disable the BRs using redirects: off - or not using redirects or paket update --redirects at all.

jackfoxy commented 8 years ago

@forki thanks.

did you try to disable binding redirects from paket.dependencies file

that's probably the piece I'm missing

forki commented 8 years ago

tbf we changed that a couple of times back and forth to make it work for more people. The current version is described at http://fsprojects.github.io/Paket/dependencies-file.html#Redirects-option

I hope we now have a good shape for this.

I'm already trying to make that framework detection working. I guess it's a good simplification for many users.

jackfoxy commented 8 years ago

@forki I might have experienced that back and forth. Simpler is usually better. There's a lot to understand. Thanks again for a great tool.

isaacabraham commented 8 years ago

@jackfoxy the truth is that redirects is a painful area at the best of times. But it's a necessary evil - without it you can often end up with runtime errors caused by missing methods or whatnot if a dependency was built against a lower version of a transient dependency than what you've pulled down.

99/100 times, leaving the redirects to paket with the redirects:on option should work fine, but if not, you can either turn them off or deal with them one-by-one.

forki commented 8 years ago

see https://github.com/fsprojects/Paket/pull/1451

jackfoxy commented 8 years ago

@isaacabraham yep, been a lot of pain in my education on redirects. My working philosophy is to keep them to the minimum necessary, as they then tell you where the pain points are.

isaacabraham commented 8 years ago

Yeah. The original code I did in Paket was very naive and extremely verbose - it did BRs even when not necessary. @mrinaldi did a lot of great work so it only puts them in when necessary now.

I've tried not using them but you run the risk of runtime errors very late in the day which is why I always turn it on. But whatever works for you!

orloffm commented 8 years ago

So, would it be possible to create some option to omit conditionals?

Or, maybe, at least, group similar references into a single conditional? We work in a corporate net40 environment and will never ever switch the project to net45. Even if I set framework: net40 in paket.dependencies, I still get all this clutter. It is not needed and can be a huge drawback when selling Paket to managers.

forki commented 8 years ago

what clutter do you get? One condition per package. An in contrast to Nuget this stays stable for most of the time since there is no version no. in path.

orloffm commented 8 years ago

But the conditions are huge, they are difficult to read compared to what would've existed without them. It's just ugly to have them. May Paket at least not forcefully recreate them if I remove them and then do some operation?

forki commented 8 years ago

Yes paket will respect your changes. But that's pretty much fighting the whole thing and way to much hassle. Tbh I don't really under the issue. How often do you look into this?

But to be constructive here: I could see a switch in the dependencies file that basically installs only one framework and without conditions. I don't think it's a good idea to use that, but I'm not completely against it. I think it's not that complicated to build. Do we have volunteers? On Feb 12, 2016 16:10, "Mikhail Orlov" notifications@github.com wrote:

But the conditions are huge, they are difficult to read compared to what would've existed without them. It's just ugly to have them. May Paket at least not forcefully recreate them if I remove them and then do some operation?

— Reply to this email directly or view it on GitHub https://github.com/fsprojects/Paket/issues/1440#issuecomment-183367116.

orloffm commented 8 years ago

install and update do not respect my changes. They always remove the plain

<ItemGroup>
    <Reference>1</Reference> <!-- just as written by Paket -->
    <Reference>2</Reference>
</ItemGroup>

and write

<Choose>
    <When>
        <ItemGroup>
            <Reference>1</Reference>
        </ItemGroup>
    </When>
</Choose>
<Choose>
    <When>
        <ItemGroup>
            <Reference>2</Reference>
        </ItemGroup>
    </When>
</Choose>

I would be 100% happy if it did not do that.

Do we have volunteers?

Fingers crossed!