Sitelet https://github.com/fsprojects/Paket/pull/2752
Skip to content

[WIP] Parallel execution of bootstrapper - #2752

Merged
forki merged 11 commits into
fsprojects:masterfrom
vbfox:parallel_bug
Oct 4, 2017
Merged

[WIP] Parallel execution of bootstrapper#2752
forki merged 11 commits into
fsprojects:masterfrom
vbfox:parallel_bug

Conversation

@vbfox

@vbfox vbfox commented Sep 10, 2017

Copy link
Copy Markdown
Contributor

I still need to fix the tests and clean the code a little but it's starting to take shape.

I have LINQPad scripts to test the behavior running lot of parallel bootstrappers downloading from github (By simulating it) and can't make it crash in it's current state.

I wanted to avoid multiple bootstrapper downloading the same version multiple times but turns out that it's harder to do well and not so important.

The core of the change is in FileSystemProxy.WaitForFileOpen that is used instead of the standard primitives that fail in such case.

@vbfox

vbfox commented Sep 10, 2017

Copy link
Copy Markdown
Contributor Author

LINQpad scripts if anyone is interested: https://gist.github.com/vbfox/1085e3c15c849000cdc3df130d7b2997

@forki

forki commented Oct 1, 2017

Copy link
Copy Markdown
Member

ping

@vbfox

vbfox commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

Woops, ah yes this PR exists 😉

@forki

forki commented Oct 2, 2017

Copy link
Copy Markdown
Member

still WIP?

@vbfox

vbfox commented Oct 2, 2017

Copy link
Copy Markdown
Contributor Author

Yes I need to fix the tests, i'll get to it, thanks for reminding me it exists :)

@vbfox

vbfox commented Oct 3, 2017

Copy link
Copy Markdown
Contributor Author

Build failures seem unrelated, but as the linux one doesnt want to start I need to test at least on one mono platform.

Currently setting up an unbutu VM, wish me luck with mono bugs.

@matthid

matthid commented Oct 3, 2017

Copy link
Copy Markdown
Member

I restarted AppVeyor.
Yes I killed travis by trying to add msbuild:

The following packages will be REMOVED:
  fsharp mono-complete mono-devel mono-roslyn
The following NEW packages will be installed:
  libunwind8 msbuild msbuild-libhostfxr msbuild-sdkresolver

I have no fucking idea why they would add a not compatible msbuild package to their release apt-source. This is complete insanity and I think I managed to reach a point where I completely lost faith in anything they do or release.

Also, we now have

 Expected: String starting with "123test

"

  But was:  "123test

\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u0000\u

Which looks a lot like a buffer overflow ;). It could be something you might have broken with this PR as it is a bootstrapper unit test (but it might very well be a mono bug as it looks a lot like it). Maybe you found a security issue ;)

@vbfox

vbfox commented Oct 3, 2017 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Nah the unit test error is mono writing text files with the byte-order mark...
I'll use a stringreader instead of trying fancy tricks, I suspect it handle this case XD

(It's only a test error, I took too much shortcuts in that test)

@forki

forki commented Oct 4, 2017

Copy link
Copy Markdown
Member

ready to merge?

@vbfox

vbfox commented Oct 4, 2017

Copy link
Copy Markdown
Contributor Author

All my tests were successful but i'd like someone to double check that everything works :)

All of my personal & professional projects have a pinned version of the bootstrapper (Locking versions FTW) so I never saw the original problem before I started testing specifically for it.

But the PR itself is finished for me, I tested on mac & linux and it works and bootstrap paket without problems.

@forki
forki requested a review from matthid October 4, 2017 07:42
@forki
forki merged commit f2fff23 into fsprojects:master Oct 4, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@matthid matthid Awaiting requested review from matthid

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@vbfox @forki @matthid