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

#3127 storage symlink feature #3128

Merged
merged 2 commits into from Mar 27, 2018

Conversation

Projects
None yet
4 participants
@cboudereau

cboudereau commented Mar 21, 2018

The impl is done
I have manually tested on mono + windows the behavior (paket local, restore, install and update) with and without the configuration / globally and on a specific package.

On a little project 5s before and 270ms after.

When you restore dotnet core packages and the nuget cache is full the restore phase is less a second instead of 20s.

I am building integration test.

@cboudereau

This comment has been minimized.

cboudereau commented Mar 21, 2018

@forki I don't know why there was an error on the Travis ci build with mono. Could you help me ? The metaproj error is a little bit weird for me. Thanks!

Microsoft Visual Studio Solution File, Format Version 12.00
# Visual Studio 15
VisualStudioVersion = 15.0.26730.0
MinimumVisualStudioVersion = 10.0.40219.1
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Paket.Core", "src/Paket.Core.preview3/Paket.Core.fsproj", "{779DA2DD-CEA0-4EC4-9DBD-2CF29C2269EA}"
Project("{6EC3EE1D-3C4E-46DD-8F32-0CC8E7565705}") = "Paket.Core", "src\Paket.Core.preview3\Paket.Core.fsproj", "{779DA2DD-CEA0-4EC4-9DBD-2CF29C2269EA}"

This comment has been minimized.

@matthid

matthid Mar 21, 2018

Member

change \ back to / and travis will likely work again...

This comment has been minimized.

@cboudereau
@matthid

The changes look reasonable to me. Obviously we need a green CI but other than that it really looks more simple than what I would have expected...

if not isLocalOverride then
match isLocalOverride, configResolved with
| true, ResolvedPackagesFolder.NoPackagesFolder -> return failwithf "paket.local in combination with storage:none is not supported"

This comment has been minimized.

@matthid

matthid Mar 21, 2018

Member

Maybe we add to that message "use storage: symlink instead" ?

This comment has been minimized.

@cboudereau
@@ -111,7 +111,7 @@ let ExtractPackage(alternativeProjectRoot, root, groupName, sources, caches, for
| LocalNuGet(path,_) as source ->
return! extractPackage caches package alternativeProjectRoot root localOverride source groupName v includeVersionInPath downloadLicense force
}

This comment has been minimized.

@matthid

matthid Mar 21, 2018

Member

maybe remove that whitespace

This comment has been minimized.

@cboudereau
@@ -203,14 +203,13 @@ module RuntimeGraph =
open System.IO
/// Downloads the given package into the nuget cache and read its runtime.json.
let getRuntimeGraphFromNugetCache root groupName (package:ResolvedPackage) =
let config = PackagesFolderGroupConfig.NoPackagesFolder
let getRuntimeGraphFromNugetCache root configO groupName (package:ResolvedPackage) =

This comment has been minimized.

@matthid

matthid Mar 21, 2018

Member

configO? Maybe just config?

This comment has been minimized.

@cboudereau
@matthid

This comment has been minimized.

Member

matthid commented Mar 21, 2018

One could argue about starting external processes, but we already do that on other places as well.

@cboudereau

This comment has been minimized.

cboudereau commented Mar 21, 2018

Yeah about external process I agree but dotnet core API are not yet ready due to case clash between claimed and FileInfo/DirectoryInfo (dotnet/corefx#25569). So I reuse the external tools to preserve OS dependent behavior.

@cboudereau cboudereau changed the title from [WIP] #3127 storage symlink feature to #3127 storage symlink feature Mar 21, 2018

cboudereau
@cboudereau

This comment has been minimized.

cboudereau commented Mar 21, 2018

@matthid I have fixed the last failing test. There is some well known tests failing on AppVeyor (as I understand according to previous merged). Is it ok for you ? Thanks in advance for your help! I appreciated it

@cboudereau

This comment has been minimized.

cboudereau commented Mar 22, 2018

Hello @matthid, do you think that the checks are done (even if there is well known failing tests ? I am currently investigating why those tests failed)

@matthid

This comment has been minimized.

Member

matthid commented Mar 22, 2018

@forki is this ready? is the test failure related?

@cboudereau

This comment has been minimized.

cboudereau commented Mar 22, 2018

Here is the diagnostic :

" Lowercase package names in package cache: old csproj, packages folder enabled" is failing due to FSharp.Core binding redirects (4.4.1 -> 4.4.3) of the FakeLib.dll logger. By disabling logger the test works fine (Fake.MSBuildHelper.MSBuildLoggers <- [] //There is a fsharp.core binding redirect issue on the FakeLib.dll logger)

"#2335 should install deps from different groups when using conditions" : this one failed due to new framework versions.

"#2294 pin NETStandard.Library = 1.6.0 Strategy Workaround" : paket timeout after 7mn
"2294 Cannot pin NETStandard.Library = 1.6.0" : paket timeout after 7mn

I am running with a timeout infinite the #2294 but test are still running for 20mn.

If I compare the build tests between mine and the last merged, tests are on the same status.
https://ci.appveyor.com/project/paket/paket/build/0.0.1.347

I am adding a new commit to fix 2 tests.

@cboudereau

This comment has been minimized.

cboudereau commented Mar 22, 2018

@matthid by fixing those 2 tests (not flaky), all CI are done. @forki could you check if all is ok please ?

Thank a lots for your help!

cboudereau
@forki

This comment has been minimized.

Member

forki commented Mar 27, 2018

I wonder if we can make that the default eventually?

@thinkbeforecoding

This comment has been minimized.

thinkbeforecoding commented Mar 27, 2018

Probably. Let's make it opt-in for now, and make it default later when we are sure there are no problematic edge cases.

@forki forki merged commit 7c62fa0 into fsprojects:master Mar 27, 2018

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@forki

This comment has been minimized.

Member

forki commented Mar 27, 2018

I get:

image

@cboudereau

This comment has been minimized.

cboudereau commented Mar 27, 2018

Which OS/Environment ? (as I understand it is a permission issue). We are currently running it on windows without highest privilege. Same on bash ubuntu for windows.

@forki

This comment has been minimized.

Member

forki commented Mar 27, 2018

running on windows in non-admin terminal

@cboudereau

This comment has been minimized.

cboudereau commented Mar 27, 2018

Have you checked policies around mklink : Local Policies\User Rights Assignment\Create symbolic links ??

@matthid

This comment has been minimized.

Member

matthid commented Mar 27, 2018

So no default for now ;)

@matthid

This comment has been minimized.

Member

matthid commented Mar 27, 2018

But it is probably fine as it is. I don't think it will get any better than this.

@thinkbeforecoding

This comment has been minimized.

thinkbeforecoding commented Mar 27, 2018

Are you sure there's not some process locking the dll ?

@thinkbeforecoding

This comment has been minimized.

thinkbeforecoding commented Mar 27, 2018

We reproduce the problem on our CI server on a build.. checking what's happening.

@cboudereau

This comment has been minimized.

cboudereau commented Mar 27, 2018

@forki

This comment has been minimized.

Member

forki commented Mar 27, 2018

@cboudereau can you please send a PR to the docs with details? also a error message that points to the docs would be nice

@cboudereau

This comment has been minimized.

cboudereau commented Mar 27, 2018

@cboudereau

This comment has been minimized.

cboudereau commented Mar 27, 2018

done #3134

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