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

Do not parse DNX stuff as dotnetcoreapp - fixes #3544 #3545

Merged
merged 1 commit into from
Apr 10, 2019
Merged

Do not parse DNX stuff as dotnetcoreapp - fixes #3544 #3545

merged 1 commit into from
Apr 10, 2019

Conversation

forki
Copy link
Member

@forki forki commented Apr 8, 2019

No description provided.

@forki forki force-pushed the i3544 branch 10 times, most recently from 521fade to eaad940 Compare April 9, 2019 15:10
@forki forki requested a review from matthid April 10, 2019 07:04
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.

Can you briefly explain what the root cause is and what the fix here does. Maybe it's because I just woke up but I don't get it (I looked at the referenced issue #3544 but I'm not exactly sure why removing a framework would break the resolution)

@@ -372,7 +372,7 @@ module LockFileParser =
| e ->
try InstallSettings.Parse(true, "framework: " + settingsPart) // backwards compatible
with e2 ->
raise <| AggregateException(sprintf "failed to parse line '%s'" line, e, e2)
raise <| AggregateException(sprintf "failed to parse line '%s'. Message: %s" line e2.Message, e, e2)
Copy link
Member

Choose a reason for hiding this comment

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

There shouldn't be a need for these changes as the error reporter at the end outputs all inner exception. If there is a need for these kind of changes we failed to propagate error information correctly and we should fix that instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think this was my mistake. I didn't read the message top to bottom and so I thought this would be needed,

@forki
Copy link
Member Author

forki commented Apr 10, 2019

my latest fix which tried to remove DNX was a bit too lazy. I just set it equal with dotnet core 1.
This leads to #3544 where it resolves the wrong stuff. This one here is more correct since it actually just ignores that crap.

@matthid
Copy link
Member

matthid commented Apr 10, 2019

So you removed the stuff in #3530 and issue X was opened, then you readded dnx as netcoreapp1.0 via PR/commit Y, and then #3544 appeared, and this PR tried to fix that.
Can you link X and Y?

@forki
Copy link
Member Author

forki commented Apr 10, 2019

I set DNX as dotnetcore 1 in #3530.
Then #3544 appeared and this is the final fix

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.

Ah OK, we kept it because of lockfile compatibility. Makes sense.

Only one idea: Should we warn when the lock-file contains "Unsupported"?
Anyway, we should try and see if that works. Maybe we should add a test ;)

@forki
Copy link
Member Author

forki commented Apr 10, 2019

we don't warn. with next paket install the crap gets nuked anyway...

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.

2 participants