Fix RestrictionsChanged Detection #3464

lust4life · 2018-12-22T08:08:55Z

// the requirement's setting
{ImportTargets = None;
    FrameworkRestrictions = AutoDetectFramework;
    OmitContent = None;
    IncludeVersionInPath = None;
    LicenseDownload = None;
    ReferenceCondition = None;
    CreateBindingRedirects = None;
    EmbedInteropTypes = None;
    CopyLocal = None;
    SpecificVersion = None;
    StorageConfig = Some NoPackagesFolder;
    Excludes = [];
    Aliases = map [];
    CopyContentToOutputDirectory = None;
    GenerateLoadScripts = None;}

// the resolved package 
// Install Settings
{ImportTargets = None;
    FrameworkRestrictions = ExplicitRestriction true;                                                                                                       
    OmitContent = None;                                                                                                                                     
    IncludeVersionInPath = None;
    LicenseDownload = None;                                                                                                                                 
    ReferenceCondition = None;                                                                                                                              
    CreateBindingRedirects = None;
    EmbedInteropTypes = None;                                                                                                                               
    CopyLocal = None;
    SpecificVersion = None;                                                                                                                                 
    StorageConfig = None;
    Excludes = [];                                                                                                                                          
    Aliases = map [];
    CopyContentToOutputDirectory = None;
    GenerateLoadScripts = None;};

//    GroupSettings =
{ImportTargets = None;
      FrameworkRestrictions =
       ExplicitRestriction
         || (== net461) (== netcoreapp2.0) (== netcoreapp2.2) (== netstandard2.0);
      OmitContent = None;
      IncludeVersionInPath = None;
      LicenseDownload = None;
      ReferenceCondition = None;
      CreateBindingRedirects = None;
      EmbedInteropTypes = None;
      CopyLocal = None;
      SpecificVersion = None;
      StorageConfig = Some NoPackagesFolder;
      Excludes = [];
      Aliases = map [];
      CopyContentToOutputDirectory = None;
      GenerateLoadScripts = None;};

// Settings =
{ImportTargets = None;
      FrameworkRestrictions =
       ExplicitRestriction
         || (== net461) (== netcoreapp2.0) (== netcoreapp2.2) (== netstandard2.0);
      OmitContent = None;
      IncludeVersionInPath = None;
      LicenseDownload = None;
      ReferenceCondition = None;
      CreateBindingRedirects = None;
      EmbedInteropTypes = None;
      CopyLocal = None;
      SpecificVersion = None;
      StorageConfig = Some NoPackagesFolder;
      Excludes = [];
      Aliases = map [];
      CopyContentToOutputDirectory = None;
      GenerateLoadScripts = None;};}

lust4life · 2018-12-22T17:12:22Z

@matthid the impact here is :
when we use config like below in packet.dependencies

storage: none
framework: auto-detect

run dotnet build , it will always shows a trace warning like

  paket.dependencies and paket.lock are out of sync in E:\git\test\go.
  Please run 'paket install' or 'paket update' to recompute the paket.lock file.
  Changes were detected for Main/FSharp.Core
      - RestrictionsChanged
  Starting restore process.

although it will only occurs the first time after we generate a restore cache in xx/obj/ folder, but when in ci case, it will always show a warning message, if we have many packages, it will generate warning message for every packages. and using paket install or paket update here doesn't help.

e.g.

new project
paket.dependencies

source https://api.nuget.org/v3/index.json
storage: none
framework: auto-detect
nuget FSharp.Core

run dotnet build

lust4life · 2018-12-22T17:19:04Z

And if this is not a FullRestore , it will give this warning message always.
e.g. run paket restore --project xxx.fsproj

matthid · 2018-12-22T17:27:30Z

@lust4life Thanks for clarifying and the fix. I'd like to see a unit test for this (I think you can copy one of mine and edit them from the PR you commented in). But in any case this looks reasonable (without me looking at the details)

lust4life · 2018-12-24T06:34:21Z

@matthid test added.

matthid · 2018-12-24T09:20:50Z

Ok but the test didn't fail before the change, correct? I feel like a unit test might be easier to realize (assert that the settings are as they should be). The other way would be to assert that paket doesn't emit any warning?

lust4life · 2018-12-24T11:21:40Z

the test ( #3464 Paket restore (not full restore) should not warn RestrictionsChanged when using auto-detect with storage none) do fail before the change. since it will generate a warning message which should not be. not very familiar with paket's source code, could you tell me how to assert paket doesn't emit any warning, seems this warning message are publish to a event through traceWarn

lust4life · 2018-12-24T11:25:40Z

since we need compare a lock file and the origin dependencies file, mock a lock file's content (in unit test style) which seems not very robust when paket change its lock content's generate strategy. what do you think ?

lust4life · 2019-01-07T06:07:19Z

@matthid ping

matthid · 2019-01-07T07:51:03Z

Afaik warnings don't make integration tests fail?

lust4life · 2019-01-07T17:06:29Z

Are you suggesting a unit test to cover this situation ？i think the difference here is how to generate a lock file : by real service (integration test) or by fake one (unit test).

The problem here is not about whether the warning will cause the test to fail. It will generate inappropriate warnings.

and it give tips : paket.dependencies and paket.lock are out of sync in %s.%sPlease run 'paket install' or 'paket update' to recompute the paket.lock file. re-run paket install will not solve this problem.

matthid · 2019-01-07T17:13:52Z

All I'm saying is that the test is "green" before and after the change even if it emits a warnings before and none after the change. Afaik there is nothing in place to "check" if the warning is there or not. Therefore there is no benefit in the test as it is now.

Yes my suggestion would be to either change the test in a way to check if the warning was emitted and fail the test or use a unit-test instead (which I'd argue is easier because then you can "test" that the "settings object" is correct instead of testing the output)

lust4life · 2019-01-08T04:41:08Z

@matthid got you, fixed.

lust4life · 2019-01-10T10:38:02Z

@matthid ping, I don't mean to rush you, can you do a release for this pr ? since this warning message will cause our ci pipeline failed (too many log message, travis will kill the build job). or is there anything I can help ? 🙂

matthid

Looks good to me now. Thanks for going all the way :)

matthid · 2019-01-10T11:33:19Z

Not sure if the failing test is related, doesn't look like it.

lust4life · 2019-01-11T11:07:54Z

@matthid @forki thx.

This was referenced Dec 22, 2018

add aspnetcore adapter and samples causiq/logary#381

Merged

Make packages folder optional #2638

Merged

lust4life added a commit to lust4life/Paket that referenced this pull request Dec 24, 2018

add integration test for fsprojects#3464

517abd5

lust4life added 4 commits January 8, 2019 12:11

compare with originalPackage.settings not resolved.settings

f900b64

add integration test for fsprojects#3464

4e3b746

add ut for storage none when findNuGetChangesInDependenciesFile

dbdd79f

revert integrationtests using unit test

0e5632e

lust4life force-pushed the master branch from 517abd5 to 0e5632e Compare January 8, 2019 04:39

matthid approved these changes Jan 10, 2019

View reviewed changes

forki merged commit 0e5632e into fsprojects:master Jan 10, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix RestrictionsChanged Detection #3464

Fix RestrictionsChanged Detection #3464

lust4life commented Dec 22, 2018 •

edited

Loading

lust4life commented Dec 22, 2018

lust4life commented Dec 22, 2018 •

edited

Loading

matthid commented Dec 22, 2018

lust4life commented Dec 24, 2018

matthid commented Dec 24, 2018

lust4life commented Dec 24, 2018 •

edited

Loading

lust4life commented Dec 24, 2018 •

edited

Loading

lust4life commented Jan 7, 2019

matthid commented Jan 7, 2019

lust4life commented Jan 7, 2019

matthid commented Jan 7, 2019

lust4life commented Jan 8, 2019

lust4life commented Jan 10, 2019

matthid left a comment

matthid commented Jan 10, 2019

lust4life commented Jan 11, 2019

Fix RestrictionsChanged Detection #3464

Fix RestrictionsChanged Detection #3464

Conversation

lust4life commented Dec 22, 2018 • edited Loading

lust4life commented Dec 22, 2018

lust4life commented Dec 22, 2018 • edited Loading

matthid commented Dec 22, 2018

lust4life commented Dec 24, 2018

matthid commented Dec 24, 2018

lust4life commented Dec 24, 2018 • edited Loading

lust4life commented Dec 24, 2018 • edited Loading

lust4life commented Jan 7, 2019

matthid commented Jan 7, 2019

lust4life commented Jan 7, 2019

matthid commented Jan 7, 2019

lust4life commented Jan 8, 2019

lust4life commented Jan 10, 2019

matthid left a comment

Choose a reason for hiding this comment

matthid commented Jan 10, 2019

lust4life commented Jan 11, 2019

lust4life commented Dec 22, 2018 •

edited

Loading

lust4life commented Dec 22, 2018 •

edited

Loading

lust4life commented Dec 24, 2018 •

edited

Loading

lust4life commented Dec 24, 2018 •

edited

Loading