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

[WIP] Parallel execution of bootstrapper #2752

Merged
merged 11 commits into from
Oct 4, 2017
Merged

[WIP] Parallel execution of bootstrapper #2752

merged 11 commits into from
Oct 4, 2017

Conversation

vbfox
Copy link
Contributor

@vbfox vbfox commented Sep 10, 2017

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
Copy link
Contributor Author

vbfox commented Sep 10, 2017

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

@forki
Copy link
Member

forki commented Oct 1, 2017

ping

@vbfox
Copy link
Contributor Author

vbfox commented Oct 2, 2017

Woops, ah yes this PR exists 😉

@forki
Copy link
Member

forki commented Oct 2, 2017

still WIP?

@vbfox
Copy link
Contributor Author

vbfox commented Oct 2, 2017

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

@vbfox
Copy link
Contributor Author

vbfox commented Oct 3, 2017

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
Copy link
Member

matthid commented Oct 3, 2017

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
Copy link
Contributor Author

vbfox commented Oct 3, 2017 •
edited
Loading

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
Copy link
Member

forki commented Oct 4, 2017

ready to merge?

@vbfox
Copy link
Contributor Author

vbfox commented Oct 4, 2017

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.

None yet
3 participants
@vbfox @forki @matthid