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

Fix fast restore #3608

Merged
merged 6 commits into from
Jul 3, 2019
Merged

Fix fast restore #3608

merged 6 commits into from
Jul 3, 2019

Conversation

matthid
Copy link
Member

@matthid matthid commented Jul 1, 2019

This has regressed countless times:

The last regression was due to the change of the caching file to only contain hashes. This is tricky to fix in msbuild, but there is https://docs.microsoft.com/en-us/visualstudio/msbuild/getfilehash-task?view=vs-2019 which seems to produce the exact same hash we already generate in C# (when using SHA256 in hex format).
I guess this also replaces #2870, lets see if this actually works for everyone, if not we are doomed ;)

Therefore we:

@matthid
Copy link
Member Author

matthid commented Jul 1, 2019

/cc @TheAngryByrd Please check if this regresses on #2796

@matthid matthid changed the title [WIP] Fix fast restore Fix fast restore Jul 1, 2019
@TheAngryByrd
Copy link
Contributor

Seems to be working for me!

Only issue I seemed to have is having an older FAKE version causes paket.restore.cached to be in a old format and give this new Targets file errors. Might need to recommend people update their FAKE version if they hit this error

.paket/Paket.Restore.targets(101,13): error MSB4184: The expression ""System.String[]".GetValue(1)" cannot be evaluated. Index was outside the bounds of the array.

@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

I think we should fix that, if we cannot read the cache file we should probably call restore and consider it outdated

@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

Seems the testsuite agrees ;)

@forki
Copy link
Member

forki commented Jul 2, 2019

is this ready?

@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

Should be ready if it turns green. I addressed the failing tests and the issue @TheAngryByrd pointed out (including a test for this scenario as none existed)

Notice that this also fixes fsprojects/FAKE#2348 which apparently is a feature now in the SAFE-Stack. Instead of adding an integration test into FAKE, I think it should be enough that we now have one here, time will tell.

@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

I accidentally pushed into origin and then deleted the branch, therefore we now have 4 instead of 2 CI builds and two are red because they can't find the branch :)

@forki
Copy link
Member

forki commented Jul 2, 2019

@matthid you always find novel ways to confuse me ;-)

@forki forki closed this Jul 2, 2019
@forki forki reopened this Jul 2, 2019
@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

That's why I added a comment ;).
push origin is just to much muscle memory ;)

@forki
Copy link
Member

forki commented Jul 2, 2019 via email

@baronfel
Copy link
Contributor

baronfel commented Jul 2, 2019

That's what I do, too terrified I'll flub things up by happenstance

@matthid
Copy link
Member Author

matthid commented Jul 2, 2019

Had to fix another test anyway, so back to normal ;)

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.

4 participants