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

[WIP] Parallel execution of bootstrapper #2752

Merged
merged 11 commits into from Oct 4, 2017

Conversation

Projects
None yet
3 participants
@vbfox
Contributor

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

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Sep 10, 2017

Contributor

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

Contributor

vbfox commented Sep 10, 2017

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

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 1, 2017

Member

ping

Member

forki commented Oct 1, 2017

ping

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 2, 2017

Contributor

Woops, ah yes this PR exists 😉

Contributor

vbfox commented Oct 2, 2017

Woops, ah yes this PR exists 😉

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 2, 2017

Member

still WIP?

Member

forki commented Oct 2, 2017

still WIP?

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 2, 2017

Contributor

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

Contributor

vbfox commented Oct 2, 2017

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

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 3, 2017

Contributor

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.

Contributor

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

This comment has been minimized.

Show comment
Hide comment
@matthid

matthid Oct 3, 2017

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 ;)

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

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 3, 2017

Contributor

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)

Contributor

vbfox commented Oct 3, 2017

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)

vbfox added some commits Oct 3, 2017

@forki

This comment has been minimized.

Show comment
Hide comment
@forki

forki Oct 4, 2017

Member

ready to merge?

Member

forki commented Oct 4, 2017

ready to merge?

@vbfox

This comment has been minimized.

Show comment
Hide comment
@vbfox

vbfox Oct 4, 2017

Contributor

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.

Contributor

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 Oct 4, 2017

@forki forki merged commit f2fff23 into fsprojects:master Oct 4, 2017

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment