search

sillsdev / l10nsharp

A .NET localization library for Windows Forms applications.
3 stars 10 forks source link

Update L10NSharp build to download NuGet packages via msbuild #15

Closed StephenMcConnel closed 7 years ago

StephenMcConnel commented 7 years ago

I copied the NuGet.targets file from BloomDesktop in case you were wondering about the changes in that file.

This change is Reviewable

andrew-polk commented 7 years ago
:lgtm:

Reviewed 2 of 2 files at r1. Review status: all files reviewed at latest revision, all discussions resolved.

Comments from Reviewable

ermshiperete commented 7 years ago

I'm not sure what motivated this change. Wasn't restoring packages already possible before when called from the command line: xbuild /t:Test build/L10NSharp.proj or xbuild /t:RestorePackages build/L10NSharp.proj?

This change introduces the deprecated MSBuild-integrated restore which IIRC has the disadvantage that it'll always try to restore the packages when building. This can slow down things if you do a lot of builds and the packages didn't change. IMO it would be better to use one of the other approaches for package restore listed in the nuget docs.

StephenMcConnel commented 7 years ago

Okay, I see that those targets with that project file do work on Linux. But they are not documented, so I didn't know they existed, and I lost 2-3 hours of work yesterday trying to figure something out that would work. And msbuild on Windows worked and restored the packages with building with the solution, making me think that Linux wasn't working right. We need a consistent approach across our projects for using NuGet. I fully understand the disadvantage of having an integrated restore that runs every time you build, but that's how Bloom has been working and it's the first project I've worked on that uses NuGet.

Eberhard, do you have any recommendation for which approach our projects should use?

ermshiperete commented 7 years ago

@StephenMcConnel Sorry you lost so much time trying to get a working solution.

I'd recommend to rely on the package restore in the IDE (Visual Studio, MD, and Rider can all restore the packages when loading the solution). For the cases where we don't use an IDE (like when building from the command line) I'd import the NuGet.targets file into the *.proj file and make the Build target depend on RestorePackages.

By the way, tried to introduce this approach in Bloom (commit d541fa27), but unfortunately it got later reverted.

This approach should already work in L10NSharp (without the modifications in this commit). If you decide to go that route and it doesn't work, please let me know so that we can get it working. It's successfully used in several other projects, but maybe there's some edge case or slightly different environment if it doesn't work as is in this project.