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.

Sign up for GitHub

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

Jump to bottom

GH1743: Implements functionality for downloading NuGet packages #1768

Merged
merged 1 commit into from
Sep 12, 2017
Merged

GH1743: Implements functionality for downloading NuGet packages #1768

merged 1 commit into from
Sep 12, 2017

Conversation

bjorkstromm
Copy link
Member

@bjorkstromm bjorkstromm commented Aug 29, 2017 •
edited
Loading

TODO:

  • Make this opt-in through configuration param.
  • Add integration tests.
  • Opt-in Load Dependencies (both config and directive)

devlead
devlead reviewed Aug 29, 2017
View reviewed changes
src/Cake.NuGet/Install/NuGetProjectContext.cs Outdated
{
switch (level)
{
case MessageLevel.Info:
Copy link
Member

Choose a reason for hiding this comment

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

It's a bit chatty now by default, I wonder if we should have Verbose for info.

gep13
gep13 reviewed Aug 29, 2017
View reviewed changes
src/Cake.NuGet/Install/NuGetSourceRepositoryProvider.cs Outdated
{
private static readonly string[] _defaultSources =
{
"https://api.nuget.org/v3/index.json"
Copy link
Member

Choose a reason for hiding this comment

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

@mholo65 should this come from the cake.config file?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, I think it should.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yes it should!

bjorkstromm
bjorkstromm commented Sep 3, 2017
View reviewed changes
src/Cake.NuGet.Tests/Unit/NuGetModuleTests.cs

// Then
fixture.Registrar.Received(1).RegisterType<Install.NuGetPackageInstaller>();
fixture.Builder.Received(1).As<INuGetPackageInstaller>();
Copy link
Member Author

Choose a reason for hiding this comment

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

@cake-build/cake-team can anyone tell me why this test is failing here?

Copy link
Member

Choose a reason for hiding this comment

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

see above comment

@bjorkstromm bjorkstromm changed the title [WIP] GH1743: Implements functionality for downloading NuGet packages GH1743: Implements functionality for downloading NuGet packages Sep 3, 2017
devlead
devlead requested changes Sep 7, 2017
View reviewed changes
Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

@mholo65 Could you please sort the merge conflicts and rebase against latest develop.

@bjorkstromm
Copy link
Member Author

@devlead rebased.

devlead
devlead requested changes Sep 7, 2017
View reviewed changes
src/Cake.NuGet.Tests/Unit/NuGetModuleTests.cs Outdated
public void Should_Register_The_In_Process_NuGet_Package_Installer_If_Set_In_Configuration()
{
// Given
var fixture = new NuGetModuleFixture<NuGetPackageInstaller>();
Copy link
Member

Choose a reason for hiding this comment

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

This should be

var fixture = new NuGetModuleFixture<Install.NuGetPackageInstaller>();

Copy link
Member Author

Choose a reason for hiding this comment

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

Ofc, stupid me looking at wrong place! Thank you!

src/Cake.NuGet.Tests/Unit/NuGetModuleTests.cs

// Then
fixture.Registrar.Received(1).RegisterType<Install.NuGetPackageInstaller>();
fixture.Builder.Received(1).As<INuGetPackageInstaller>();
Copy link
Member

Choose a reason for hiding this comment

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

see above comment

@bjorkstromm
Copy link
Member Author

@devlead fixed

devlead
devlead approved these changes Sep 9, 2017
View reviewed changes
Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

LGTM 👍 I'll take it for a final spin, if all good I'll mere this one.

devlead
devlead requested changes Sep 9, 2017
View reviewed changes
Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

Got an exception on my Linux machine will investigate on Windows and see if I get same error and then rerun tests on Linux if all's good on WIndows

Error: System.ArgumentException: Path cannot be empty.
Parameter name: path
  at Cake.Core.IO.Path..ctor (System.String path) <0x400fd040 + 0x0048f> in <filename unknown>:0
  at Cake.Core.IO.FilePath..ctor (System.String path) <0x400fc030 + 0x00013> in <filename unknown>:0
  at Cake.NuGet.Install.NugetFolderProject.GetFiles (Cake.Core.IO.DirectoryPath directoryPath, Cake.Core.Packaging.PackageReference packageReference, PackageType type) <0x402bb980 + 0x002af> in <filename unknown>:0
  at Cake.NuGet.Install.NuGetPackageInstaller.Install (Cake.Core.Packaging.PackageReference package, PackageType type, Cake.Core.IO.DirectoryPath path) <0x401632e0 + 0x00443> in <filename unknown>:0
  at Cake.Core.Scripting.ScriptProcessor.InstallTools (IReadOnlyCollection`1 tools, Cake.Core.IO.DirectoryPath installPath) <0x40162c50 + 0x0018d> in <filename unknown>:0
  at Cake.Core.Scripting.ScriptRunner.Run (IScriptHost host, Cake.Core.IO.FilePath scriptPath, IDictionary`2 arguments) <0x40152a80 + 0x003a9> in <filename unknown>:0
  at Cake.Commands.BuildCommand.Execute (Cake.CakeOptions options) <0x40152990 + 0x00042> in <filename unknown>:0
  at Cake.CakeApplication.Run (Cake.CakeOptions options) <0x40111d70 + 0x00039> in <filename unknown>:0
  at Cake.Program.Main () <0x400c0d60 + 0x0046b> in <filename unknown>:0

@devlead
Copy link
Member

devlead commented Sep 9, 2017

@mholo65
Found one thing on Windows, the nuget client libs doesn't seem to be able to use sources with encrypted passwords.

And it doesn't respect if you place a NuGet.config in repo root/tools folder/or cake folder.

Interestingly the source it complains about is disabled, so it shouldn't be used, if i remove the disabled sources and only have

<?xml version="1.0" encoding="utf-8"?>
<configuration>
  <packageSources>
    <add key="NuGetV3" value="https://api.nuget.org/v3/index.json" />
  </packageSources>
</configuration>

Feels like an issue with the NuGet client libs.

I don't get the same errors as I got on Linux so I'll rerun the tests there if it was something temporary.

@bjorkstromm
Copy link
Member Author

@devlead exception should be fixed... Will re-investigate re-download. If nothing helps, best would be to set PackageSaveMode to NuPkg. Any objections, @gep13?

@devlead
Copy link
Member

devlead commented Sep 9, 2017

@mholo65 I can confirm exception is sorted and it now works for me the first time.
But the second time I execute Cake with tools & addins restored in place it fails with:
Error: System.AggregateException: One or more errors occurred. ---> System.InvalidOperationException: Package 'gitreleasemanager 0.5.0' is not found in the following primary source(s): '/pullrequest/tools'. Please verify all your online package sources are available (OR) package id, version are specified correctly.

@bjorkstromm
Copy link
Member Author

@devlead I blame NuGet client libs and awful xplat support when it comes to file system. This breaks support for nuspec package save mode since nuspec is stored to disk as normalized (e.g. filename to lower invariant). But then when it checks if package is installed, it expects to find nuspec in non-normalized form.

@bjorkstromm
(cake-buildGH-1743) Implements functionality for downloading NuGet pa…
ac7850b 
…ckages

- Opt-in via NuGet_UseInProcessClient
- Opt-in load dependencies via NuGet_LoadDependencies
  or add LoadDependencies as parameter in directive
- Fixes cake-build#1743
@bjorkstromm
Copy link
Member Author

@devlead @gep13 rebased. Please take it for a spin.

devlead
devlead approved these changes Sep 12, 2017
View reviewed changes
Copy link
Member

@devlead devlead left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@devlead devlead merged commit ac7850b into cake-build:develop Sep 12, 2017
@devlead
Copy link
Member

devlead commented Sep 12, 2017

@mholo65 your changes have been merged, thanks for your contribution 👍

@bjorkstromm bjorkstromm deleted the feature/GH-1743 branch September 12, 2017 18:40
@bjorkstromm bjorkstromm mentioned this pull request Sep 12, 2017
Closed
@adamhathcock adamhathcock mentioned this pull request Sep 24, 2017
Merged
5 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gep13 gep13 gep13 left review comments

@devlead devlead devlead approved these changes

@patriksvensson patriksvensson Awaiting requested review from patriksvensson

@daveaglick daveaglick Awaiting requested review from daveaglick

@agc93 agc93 Awaiting requested review from agc93

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@bjorkstromm @devlead @gep13 @dnfclas