Skip to content
This repository has been archived by the owner on May 4, 2018. It is now read-only.

Windows build broken on SMP #984

Closed
geertj opened this issue Nov 4, 2013 · 15 comments
Closed

Windows build broken on SMP #984

geertj opened this issue Nov 4, 2013 · 15 comments

Comments

@geertj
Copy link
Contributor

geertj commented Nov 4, 2013

I'm trying to compile the current master on Windows. I'm using the following configuration:

  • Windows 7, 64-bit.
  • Python 2.7.6rc1 32-bit is installed, but not in the PATH. I use set PYTHON=C:\Python27\python.exe to allow the build to find it.
  • Microsoft Visual C++ Express 2010.

When running "vcbuild.bat" from the source directory, I get the following error:

https://gist.github.com/anonymous/7299594

For some reason, build command never exits and the error message keeps repeating itself.

I can fix the build by doing a copy gyp_uv gyp_uv.py.

Since Python 2.7.6 is currently a pre-release, I also tried 2.7.5, but I got the same issue.

@saghul, this also breaks the pyuv build on WIndows.

@saghul
Copy link
Contributor

saghul commented Nov 4, 2013

Hum, this looks really weird. Nothing special changed in gyp_uv which would make it not work anymore. Also, I don't get why something from multiprocessing is used.

Unfortunately I won't have access to my Windows test machine for a few weeks in order to debug this.

@bnoordhuis
Copy link
Contributor

That looks like a python issue more than anything else. Like @saghul said, nothing really changed in gyp_uv.

@bnoordhuis
Copy link
Contributor

Forgot to mention the obvious: if rolling back to 2.7.5 fixes it, then you know for sure it's an issue with python and not libuv.

@saghul
Copy link
Contributor

saghul commented Nov 4, 2013

Apparently rolling back the Python version doesn't fix it. I know I tested this (using the Python env variable), it was added because pyuv needed it, so I'm not sure what's going on.

@geertj
Copy link
Contributor Author

geertj commented Nov 4, 2013

OK, I found the problem. My build VM has 2 virtual CPUs, and it appears that gyp is trying to do a parallel build. This requires the multiprocessing module. Because Windows doesn't have fork(), multiprocessing needs to start up a new Python and re-import "gyp_uv" as a module, which fails because it is not a python module.

One solution is to add the option "--no-parallel" when calling "gyp_uv". The other is to rename gyp_uv to gyp_uv.py. However in the latter case see http://docs.python.org/2/library/multiprocessing.html#windows for some more requirements on the main module (parallel build appears to work though).

@saghul
Copy link
Contributor

saghul commented Nov 4, 2013

I'm undecided here, should we go for the --no-parallel just to play safe?

@bnoordhuis
Copy link
Contributor

That's probably the path of least resistance unless someone feels like patching up gyp_uv to work with multiprocessing on Windows.

@tjfontaine
Copy link
Contributor

We could also pin the checkout to beca12a56c0772fc8263ad7ca12ca022b8671c1c commit, which is what the chromium builds do

@saghul
Copy link
Contributor

saghul commented Nov 4, 2013

We could do that indeed. I just don't like the idea of depending on "libfoo X.Y" and not remembering why later :-) Yeah, git archeology helps, but still.

@geertj
Copy link
Contributor Author

geertj commented Nov 5, 2013

I have done some more reading on the Windows requirements of multiprocessing, and I don't think they are so bad. The most important requirement is that the main program is importable as a module. This means that any action taken needs to be guarded with if __name__ == '__main__'. This is already the case for gyp_uv.

My recommendation would be just to support the parallel build feature. The change required is simple. Just rename gyp_uv to gyp_uv.py and change references in vcbuild.bat and android-configure to it.

I can submit a pull request if you want.

@tjfontaine
Copy link
Contributor

I'm basically fine with that idea, @bnoordhuis would we want a wrapper/transitional gyp_uv that invokes gyp_uv.py in that case or are you fine with a straight rename?

@bnoordhuis
Copy link
Contributor

Just renaming it is fine IMO.

@saghul
Copy link
Contributor

saghul commented Nov 5, 2013

If that's all it's needed then I'd go for the rename. Thanks for looking
into it @geertj!

On Tuesday, November 5, 2013, Timothy J Fontaine wrote:

I'm basically fine with that idea, @bnoordhuishttps://github.com/bnoordhuiswould we want a wrapper/transitional gyp_uv that invokes gyp_uv.py in that
case or are you fine with a straight rename?


Reply to this email directly or view it on GitHubhttps://github.com//issues/984#issuecomment-27750929
.

/Saúl
http://saghul.net | http://about.me/saghul

@tjfontaine
Copy link
Contributor

fwiw I have tested, and it is indeed working, the change would also need to happen for v0.10

bnoordhuis pushed a commit that referenced this issue Nov 5, 2013
Gyp will try a parallel build if it detect the system has >1 processor.
This functionality depends on the Python "multiprocessing" package. The
multiprocessing package on Windows requires that the top-level module is
importable as a module, see:

  http://docs.python.org/2/library/multiprocessing.html#windows

This fixes issue #984.
bnoordhuis pushed a commit that referenced this issue Nov 5, 2013
Gyp will try a parallel build if it detect the system has >1 processor.
This functionality depends on the Python "multiprocessing" package. The
multiprocessing package on Windows requires that the top-level module is
importable as a module, see:

  http://docs.python.org/2/library/multiprocessing.html#windows

This fixes issue #984.

This is a back-port of commit 2445467 from the master branch.
@bnoordhuis
Copy link
Contributor

Fixed in 2445467 and 991409e (master and v0.10.)

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

4 participants