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.

Sign up for GitHub

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

Jump to bottom

Remove dnxcore50 moniker from lock file #2813

Merged
merged 2 commits into from
Oct 2, 2017
Merged

Remove dnxcore50 moniker from lock file #2813

merged 2 commits into from
Oct 2, 2017

Conversation

forki
Copy link
Member

@forki forki commented Oct 2, 2017

fixes #2810

matthid
matthid approved these changes Oct 2, 2017
View reviewed changes
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.

Schaut gut aus. ich frage mich ob man den Filter auch in extractPlatforms reinziehen kann. Aber vermutlich eher nicht weil sonst Gruppen als Fallback Gruppen erkannt werden

src/Paket.Core/Dependencies/NuGetCache.fs
@@ -176,7 179,8 @@ let tryGetDetailsFromCache force nugetURL (packageName:PackageName) (version:Sem
if (PackageName cachedObject.PackageName <> packageName) ||
(cachedObject.Version <> version.Normalize())
then
traceVerbose (sprintf "Invalidating Cache '%s:%s' <> '%s:%s'" cachedObject.PackageName cachedObject.Version packageName.Name (version.Normalize()))
if verbose then
Copy link
Member

Choose a reason for hiding this comment

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

macht den check nicht auch traceVerbose schon?

Copy link
Member Author

Choose a reason for hiding this comment

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

yes, but the right side (sprintf ...) would be evaluated even if verbose is false.
This was serious perf problem in the past

Copy link
Member

Choose a reason for hiding this comment

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

Should we remove traceVerbose in that case ?

@forki
Copy link
Member Author

forki commented Oct 2, 2017

if we move it into extractPlatforms then we get the annoying warnings that dnxcore is not real moniker

@forki
Copy link
Member Author

forki commented Oct 2, 2017 via email

@matthid
Copy link
Member

matthid commented Oct 2, 2017

Tests need to be adapted. The current failure looks a bit suspicious, but it might be that the error message is just incomplete and everything is fine.

@forki
Copy link
Member Author

forki commented Oct 2, 2017 via email

@forki forki merged commit 7e4acae into master Oct 2, 2017
@forki forki deleted the i2810 branch October 2, 2017 12:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@matthid matthid matthid approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Remove dnxcore50 moniker from lock file
2 participants
@forki @matthid