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 2222 - improve output of the outdated warning and fix underlying bug #2223

Merged
merged 8 commits into from Apr 9, 2017

Conversation

Projects
None yet
2 participants
@matthid
Member

matthid commented Apr 9, 2017

fixes both issues in #2222.

Before

paket.dependencies and paket.lock are out of sync in C:\PROJ\FAKE.
Please run 'paket install' or 'paket update' to recompute the paket.lock file.

After

paket.dependencies and paket.lock are out of sync in C:\PROJ\FAKE.
Please run 'paket install' or 'paket update' to recompute the paket.lock file.
Changes [RestrictionsChanged] were detected in netcore/FSharp.Compiler.Service
Changes [RestrictionsChanged] were detected in netcore/FSharp.Core
Changes [RestrictionsChanged] were detected in netcore/Libuv
Changes [RestrictionsChanged] were detected in netcore/Microsoft.CodeAnalysis.Common
Changes [RestrictionsChanged] were detected in netcore/Microsoft.CodeAnalysis.CSharp
Changes [RestrictionsChanged] were detected in netcore/Microsoft.CodeAnalysis.VisualBasic
Changes [RestrictionsChanged] were detected in netcore/Microsoft.CSharp
Changes [RestrictionsChanged] were detected in netcore/Microsoft.DiaSymReader
Changes [RestrictionsChanged] were detected in netcore/Microsoft.DiaSymReader.PortablePdb
Changes [RestrictionsChanged] were detected in netcore/Microsoft.NETCore.App
Changes [RestrictionsChanged] were detected in netcore/Microsoft.NETCore.DotNetHost
Changes [RestrictionsChanged] were detected in netcore/Microsoft.NETCore.DotNetHostPolicy
Changes [RestrictionsChanged] were detected in netcore/Microsoft.NETCore.DotNetHostResolver
Changes [RestrictionsChanged] were detected in netcore/Microsoft.NETCore.Jit
Changes [RestrictionsChan...
@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Apr 9, 2017

Member

apparently the problem was that paket could not properly parse netcore10 (which it wrote itself).

Member

matthid commented Apr 9, 2017

apparently the problem was that paket could not properly parse netcore10 (which it wrote itself).

@matthid matthid changed the title from WIP fix 2222 - improve output of the outdated warning and fix underlying bug to Fix 2222 - improve output of the outdated warning and fix underlying bug Apr 9, 2017

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Apr 9, 2017

Member

Lets see what the CI has to say :)

Member

matthid commented Apr 9, 2017

Lets see what the CI has to say :)

@@ -651,7 +651,7 @@ let Resolve (getVersionsF, getPackageDetailsF, groupName:GroupName, globalStrate
let rec step (stage:Stage) (stackpack:StackPack) compatibleVersions (flags:StepFlags) =
let fuseConflicts currentConflict priorConflictSteps =
let inline fuseConflicts currentConflict priorConflictSteps =

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

@cloudRoutine Is this OK? This prevents a stack-overflow I had when debugging (compiling in DEBUG mode)

@matthid

matthid Apr 9, 2017

Member

@cloudRoutine Is this OK? This prevents a stack-overflow I had when debugging (compiling in DEBUG mode)

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

interesting. probably just because the stack size was halved due to inlining. but I'm fine with that!

@forki

forki Apr 9, 2017

Member

interesting. probably just because the stack size was halved due to inlining. but I'm fine with that!

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

Ah okey, I thought the thing is now tail recursive but the compiler is not picking it up because of debug mode. Thanks for the clarification.

@matthid

matthid Apr 9, 2017

Member

Ah okey, I thought the thing is now tail recursive but the compiler is not picking it up because of debug mode. Thanks for the clarification.

| "netcore10" | "netcoreapp10" -> Some (DotNetCore DotNetCoreVersion.V1_0)
| "netcore11" | "netcoreapp11" -> Some (DotNetCore DotNetCoreVersion.V1_1)
| "netcore10" -> Some (DotNetCore DotNetCoreVersion.V1_0)
| "netcore11" -> Some (DotNetCore DotNetCoreVersion.V1_1)

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

Note: This is in fact cleanup as we have the netcoreapp -> netcore alias defined above.

@matthid

matthid Apr 9, 2017

Member

Note: This is in fact cleanup as we have the netcoreapp -> netcore alias defined above.

let checkResponse = if failOnChecks then failwithf else traceWarnfn
if hasAnyChanges then
checkResponse "paket.dependencies and paket.lock are out of sync in %s.%sPlease run 'paket install' or 'paket update' to recompute the paket.lock file." lockFileName.Directory.FullName Environment.NewLine
for (group, package, changes) in nugetChanges do
traceWarnfn "Changes %A were detected in %s/%s" changes (group.ToString()) (package.ToString())

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

Generating a really nice output is left as an exercise to the reader :)

@matthid

matthid Apr 9, 2017

Member

Generating a really nice output is left as an exercise to the reader :)

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

yeah we can iterate on that

@forki

forki Apr 9, 2017

Member

yeah we can iterate on that

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Apr 9, 2017

Member

OK This is ready. The AppVeyor error is unrelated and on the master branch as well.

Member

matthid commented Apr 9, 2017

OK This is ready. The AppVeyor error is unrelated and on the master branch as well.

@@ -37,8 +37,8 @@
<StartWorkingDirectory>D:\temp\coreclrtest</StartWorkingDirectory>
<StartArguments>install</StartArguments>
<StartWorkingDirectory>C:\dev\src\Paket\integrationtests\scenarios\loading-scripts\dependencies-file-flag\temp</StartWorkingDirectory>
<StartArguments>install --createnewbindingfiles</StartArguments>
<StartWorkingDirectory>D:\temp\pakkit2</StartWorkingDirectory>
<StartArguments>install</StartArguments>

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

I wished this would be in different file so that we could gitignore.

@forki

forki Apr 9, 2017

Member

I wished this would be in different file so that we could gitignore.

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

I can remove that change if you want.

@matthid

matthid Apr 9, 2017

Member

I can remove that change if you want.

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

Maybe after @Krzysztof-Cieslak brings the toml file ;)

@matthid

matthid Apr 9, 2017

Member

Maybe after @Krzysztof-Cieslak brings the toml file ;)

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

no it's fine. It's more a general observation. I'm really annoyed about that part of fsproj

@forki

forki Apr 9, 2017

Member

no it's fine. It's more a general observation. I'm really annoyed about that part of fsproj

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

yeah not sure if MSbuild 15 is better in that regard. but VS can't read it anyways ;-)

@forki

forki Apr 9, 2017

Member

yeah not sure if MSbuild 15 is better in that regard. but VS can't read it anyways ;-)

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

Well maybe we soon use all VSCode with ionide (when the full .net debugger works) and we don't have this problem anymore (because VSCode uses its own json files for debugging?)

@matthid

matthid Apr 9, 2017

Member

Well maybe we soon use all VSCode with ionide (when the full .net debugger works) and we don't have this problem anymore (because VSCode uses its own json files for debugging?)

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Apr 9, 2017

Member

Good work! Thanks a lot

Member

forki commented Apr 9, 2017

Good work! Thanks a lot

@forki forki merged commit 6cb4a35 into fsprojects:master Apr 9, 2017

1 of 2 checks passed

continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
let item = operatorSplit.[3]
match FrameworkDetection.Extract(item) with
| None ->
handleError <| sprintf "Could not parse second framework of between operator '%s'. Try to update or install again or report a paket bug." item

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

@forki Just in case I cannot react fast enough and people are experiencing failures tomorrow: We can just set failImmediatly to false within the parseRestrictions function (as first operation, essentially ignoring the parameter). This way paket will log errors instead of failing directly. But more often than not this indicates a bug somewhere...

@matthid

matthid Apr 9, 2017

Member

@forki Just in case I cannot react fast enough and people are experiencing failures tomorrow: We can just set failImmediatly to false within the parseRestrictions function (as first operation, essentially ignoring the parameter). This way paket will log errors instead of failing directly. But more often than not this indicates a bug somewhere...

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

Good thanks for the warning.

@forki

forki Apr 9, 2017

Member

Good thanks for the warning.

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

Maybe I should revert the change, push a bug fix version with old code and publish this as minor version change?

@forki

forki Apr 9, 2017

Member

Maybe I should revert the change, push a bug fix version with old code and publish this as minor version change?

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

I don't think this is a new feature, I have not met a case where this actually triggered wrongly.

I encountered two cases:

  • The file was written by paket and can no longer be parsed -> paket bug (another minor increase)
  • The file was written by the user (paket.references for example in my test case) -> user error as he entered some invalid value, its just that paket never told him.

All in all I think a patch version is enough as this indeed fixes a bug (one could argue if the additional output is a new feature though...)

@matthid

matthid Apr 9, 2017

Member

I don't think this is a new feature, I have not met a case where this actually triggered wrongly.

I encountered two cases:

  • The file was written by paket and can no longer be parsed -> paket bug (another minor increase)
  • The file was written by the user (paket.references for example in my test case) -> user error as he entered some invalid value, its just that paket never told him.

All in all I think a patch version is enough as this indeed fixes a bug (one could argue if the additional output is a new feature though...)

This comment has been minimized.

@forki

forki Apr 9, 2017

Member

doing it anyways. just in case.

@forki

forki Apr 9, 2017

Member

doing it anyways. just in case.

This comment has been minimized.

@matthid

matthid Apr 9, 2017

Member

So if something belongs into a minor release its the logic required to produce the additional output and not this part ;). But that's of IMHO.

@matthid

matthid Apr 9, 2017

Member

So if something belongs into a minor release its the logic required to produce the additional output and not this part ;). But that's of IMHO.

@matthid

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Apr 9, 2017

Member

Finally the warning is gone. As always thanks for merging and releasing so quickly :)

Member

matthid commented Apr 9, 2017

Finally the warning is gone. As always thanks for merging and releasing so quickly :)

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