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.

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
Merged

#3127 storage symlink feature #3128

merged 2 commits into from
Mar 27, 2018

Conversation

cboudereau
Copy link

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
Copy link
Author

@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}"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

Oh thanks a lot! @matthid

Copy link
Member

@matthid matthid left a comment

Choose a reason for hiding this comment

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

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"
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -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
}

Copy link
Member

Choose a reason for hiding this comment

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

maybe remove that whitespace

Copy link
Author

Choose a reason for hiding this comment

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

done

@@ -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) =
Copy link
Member

Choose a reason for hiding this comment

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

configO? Maybe just config?

Copy link
Author

Choose a reason for hiding this comment

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

done

@matthid
Copy link
Member

matthid commented Mar 21, 2018

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

@cboudereau cboudereau force-pushed the master branch 2 times, most recently from 3c36f10 to d7ed41b Compare March 21, 2018 19:21
@cboudereau
Copy link
Author

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

@cboudereau cboudereau changed the title [WIP] #3127 storage symlink feature #3127 storage symlink feature Mar 21, 2018
@cboudereau
Copy link
Author

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
Copy link
Author

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
Copy link
Member

matthid commented Mar 22, 2018

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

@cboudereau
Copy link
Author

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
Copy link
Author

@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!

@forki
Copy link
Member

forki commented Mar 27, 2018

I wonder if we can make that the default eventually?

@thinkbeforecoding
Copy link
Contributor

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
@forki
Copy link
Member

forki commented Mar 27, 2018

I get:

image

@cboudereau
Copy link
Author

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
Copy link
Member

forki commented Mar 27, 2018

running on windows in non-admin terminal

@cboudereau
Copy link
Author

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

@matthid
Copy link
Member

matthid commented Mar 27, 2018

So no default for now ;)

@matthid
Copy link
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
Copy link
Contributor

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

@thinkbeforecoding
Copy link
Contributor

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

@cboudereau
Copy link
Author

cboudereau commented Mar 27, 2018 via email

@forki
Copy link
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
Copy link
Author

cboudereau commented Mar 27, 2018 via email

@cboudereau
Copy link
Author

done #3134

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants