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

fix for #2755 #2770

Merged
merged 1 commit into from Sep 17, 2017

Conversation

Projects
None yet
2 participants
@lexarchik

lexarchik commented Sep 16, 2017

try to fix #2755

  • add cleaning target folder (packages/groupname/packagename) on "force" restore
    this solve two problems: overriding .nupkg on force restore and deal with .nupkg naming problem (normalized name from ~/.nuget vs "as is" from overrided source)
  • add overriding .nupkg file in ~/.nuget when using LocalNuGet source
Lysogorskiy Aleksey
@@ -670,7 +678,7 @@ let DownloadAndExtractPackage(alternativeProjectRoot, root, isLocalOverride:bool
use _ = Profile.startCategory Profile.Category.FileIO
let parent = Path.GetDirectoryName targetFileName
if not (Directory.Exists parent) then Directory.CreateDirectory parent |> ignore
File.Copy(nupkg.FullName,targetFileName)
File.Copy(nupkg.FullName,targetFileName,true)

This comment has been minimized.

@matthid

matthid Sep 16, 2017

Member

should we leave the last parameter to false when no path is resolved (ie 'storage: none')?

@matthid

matthid Sep 16, 2017

Member

should we leave the last parameter to false when no path is resolved (ie 'storage: none')?

This comment has been minimized.

@lexarchik

lexarchik Sep 17, 2017

I'm not sure i understand how storage: none should work.
If we set File.Copy(..., overwrite) parameter to false Paket will just fail if file already exists and normally copy it otherwise.
In branch where source is not LocalNuGet we overwrite file with File.Create without any conditions. (NuGet.fs:757).

@lexarchik

lexarchik Sep 17, 2017

I'm not sure i understand how storage: none should work.
If we set File.Copy(..., overwrite) parameter to false Paket will just fail if file already exists and normally copy it otherwise.
In branch where source is not LocalNuGet we overwrite file with File.Create without any conditions. (NuGet.fs:757).

This comment has been minimized.

@matthid

matthid Sep 17, 2017

Member

The only thing we need to watch out for is that we never write into the cache (~/.nuget) when a local override is specified, this is more than invalid and leads to hard do diagnose errors in combination with NuGet. So we just need to ensure that this cannot ever happen. This mistake is easily introduced now, because storage: none means we use ~/.nuget instead of the packages directory.

This is what I'm a bit worried about.

I think this cannot happen, because we "throw" on the caller side of this function if force and storage: none are given the same time. storage: none basically is given when configResolved.Path is None. I suggest we check and throw in this function as well when we want to fix the bug like this.

I hope this makes sense. Thanks for taking care of this.

@matthid

matthid Sep 17, 2017

Member

The only thing we need to watch out for is that we never write into the cache (~/.nuget) when a local override is specified, this is more than invalid and leads to hard do diagnose errors in combination with NuGet. So we just need to ensure that this cannot ever happen. This mistake is easily introduced now, because storage: none means we use ~/.nuget instead of the packages directory.

This is what I'm a bit worried about.

I think this cannot happen, because we "throw" on the caller side of this function if force and storage: none are given the same time. storage: none basically is given when configResolved.Path is None. I suggest we check and throw in this function as well when we want to fix the bug like this.

I hope this makes sense. Thanks for taking care of this.

This comment has been minimized.

@lexarchik

lexarchik Sep 17, 2017

Yes, thanks, i now understand the problem.

I can change commit - add check in download function, but such check already in 50 lines above - NuGet.fs:629 and download can't be called if this check fails.

We already have a test that assert that "local" package will not be copied to cache - "#1633 paket.local local source override"

So, maybe there is no need for additional check?

@lexarchik

lexarchik Sep 17, 2017

Yes, thanks, i now understand the problem.

I can change commit - add check in download function, but such check already in 50 lines above - NuGet.fs:629 and download can't be called if this check fails.

We already have a test that assert that "local" package will not be copied to cache - "#1633 paket.local local source override"

So, maybe there is no need for additional check?

This comment has been minimized.

@matthid

matthid Sep 17, 2017

Member

ah yes sorry

@matthid

matthid Sep 17, 2017

Member

ah yes sorry

@matthid matthid merged commit 2ee54f5 into fsprojects:master Sep 17, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details

@lexarchik lexarchik deleted the lexarchik:i2755 branch Sep 18, 2017

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