GeezerVison

The World Wide Web, restored to 800 pixels of glory.

GeezerVison is showing github.com — bpo-40280: Add build target for Emscripten/node.js by emmatyping · Pull Request #30495 · python/cpython

Conversation

emmatyping commented Jan 9, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link

Copy Markdown

Member

This adds a "python.js" target to the Makefile. We enable the raw
node.js filesystem mode as suggested by @thomasballinger as it will make running
the test suite under node easier, c.f.
emmatyping/python-wasm#36.

The wasm_asset.py script is updated to include the test module. Care is
taken to skip compiling the Python files that are examples of bad syntax
or encoding.

This also turns on memory growth as early into the test suite I was
running into OOMs.

cc @tiran

~~This requires my other PR to work of course #30494~~ (This is now merged)

https://bugs.python.org/issue40280

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

thomasballinger reacted with hooray emoji

All reactions

🎉 1 reaction


          bpo-40280: Add build target for Emscripten/node.js

717c09b

This adds a "python.js" target to the Makefile. We enable the raw
node fs mode as suggested by @thomasballinger as it will make running
the test suite under node easier, c.f.
emmatyping/python-wasm#36.

The wasm_asset.py script is updated to include the test module. Care is
taken to skip compiling the Python files that are examples of bad syntax
or encoding.

This also turns on memory growth as early into the test suite I was
running into OOMs.

the-knights-who-say-ni added the CLA signed label

Jan 9, 2022

bedevere-bot added the awaiting review label

Jan 9, 2022

emmatyping commented Jan 9, 2022

Copy link

Copy Markdown

Member Author

I'm unsure if this needs a NEWS entry, I'd be happy to add one if so.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.


          Merge branch 'main' of github.com:python/cpython into nodejs

5492cf8

This was referenced Jan 9, 2022

Do not define unsupported IPPROTO_SCTP emscripten-core/emscripten#15933

Open

Results of running the regression test suite on nodejs emmatyping/python-wasm#41

Closed

tiran reviewed

Jan 10, 2022

View reviewed changes

Comment thread

Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Tools/wasm/wasm_assets.py Resolved

Uh oh!

There was an error while loading. Please reload this page.


          Update Makefile.pre.in

a862bce

Co-authored-by: Christian Heimes <[email protected]>

tiran added the skip news label

Jan 10, 2022

emmatyping added 4 commits

January 10, 2022 09:29


          Change target name to python-rawfs.js

6d88ce6


          Remove special casing for test files

3ee5687


          Merge branch 'nodejs' of github.com:ethanhs/cpython into nodejs

aadbd05


          Fix threading for nodejs

ee99490

emmatyping commented

Jan 11, 2022

View reviewed changes

Comment thread

Makefile.pre.in

		CXX= @CXX@
		MAINCC= @MAINCC@
		LINKCC= @LINKCC@
		LINKCC_BUILDPYTHON = @LINKCC_BUILDPYTHON@

emmatyping Jan 11, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the least intrusive way I could think of to customize the link step only for building the python.wasm executable...

If you know of a better way, please do tell :)

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tiran Jan 11, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh! The ugly hacks are piling up. Maybe it's easier to not use ac_cv_pthread and to add -pthread to CFLAGS or CFLAGS_NODIST after all?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

emmatyping Jan 11, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unfortunately then it would be passed to the $(BUILDPYTHON) target, and standalone wasm doesn't support -pthread

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tiran reviewed

Jan 11, 2022

View reviewed changes

Comment thread

Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/socketmodule.c Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Modules/socketmodule.c

		PyModule_AddIntMacro(m, IPPROTO_VRRP);
		#endif
		#ifdef IPPROTO_SCTP
		#if defined(IPPROTO_SCTP) && !defined(__EMSCRIPTEN__)

tiran Jan 11, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I found a better way to deal with the issue. I guess you did the same mistake as I and added the skipif call after the check for AIX? That doesn't work because @requireSocket("AF_INET", "SOCK_STREAM", "IPPROTO_SCTP") tries to create an IPPROTO_SCTP socket and decorators are executed in reverse order!

This works:

--- a/Lib/test/test_socket.py
+++ b/Lib/test/test_socket.py
@@ -43,6 +43,7 @@
 
 VSOCKPORT = 1234
 AIX = platform.system() == "AIX"
+EMSCRIPTEN = sys.platform == "Emscripten"
 
 try:
     import _socket
...

 @requireAttrs(socket.socket, "sendmsg")
[email protected](AIX, "IPPROTO_SCTP: [Errno 62] Protocol not supported on AIX")
 @requireSocket("AF_INET", "SOCK_STREAM", "IPPROTO_SCTP")
[email protected](AIX, "IPPROTO_SCTP: [Errno 62] Protocol not supported on AIX")
[email protected](EMSCRIPTEN, "IPPROTO_SCTP: aborts on Emscripten")
 class SendmsgSCTPStreamTest(SendmsgStreamTests, SendrecvmsgSCTPStreamTestBase):

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

emmatyping Jan 11, 2022

Copy link

Copy Markdown

Member Author

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see you've opened #30538 so I'll remove this change.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

tiran Jan 11, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

#30538 does not work ... it's still running the decorator and crashing node.js

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

emmatyping reacted with confused emoji

All reactions

😕 1 reaction

Comment thread

Makefile.pre.in

		CXX= @CXX@
		MAINCC= @MAINCC@
		LINKCC= @LINKCC@
		LINKCC_BUILDPYTHON = @LINKCC_BUILDPYTHON@

tiran Jan 11, 2022

Copy link

Copy Markdown

Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Meh! The ugly hacks are piling up. Maybe it's easier to not use ac_cv_pthread and to add -pthread to CFLAGS or CFLAGS_NODIST after all?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

All reactions

Comment thread

Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

Comment thread

Makefile.pre.in Resolved

Uh oh!

There was an error while loading. Please reload this page.

emmatyping and others added 4 commits

January 11, 2022 09:15


          Update Makefile.pre.in

3a0dd1c

Co-authored-by: Christian Heimes <[email protected]>


          Update Makefile.pre.in

faf52a4

Co-authored-by: Christian Heimes <[email protected]>


          Update Makefile.pre.in

bdcaa48

Co-authored-by: Christian Heimes <[email protected]>


          Update Modules/socketmodule.c

Co-authored-by: Christian Heimes <[email protected]>

brettcannon self-requested a review

January 12, 2022 01:08

emmatyping commented Jan 12, 2022

Copy link

Copy Markdown

Member Author

I think the important bits of this ended up in #30552 so this can be closed.

All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.

emmatyping closed this

Jan 12, 2022

SimonBiggs mannequin mentioned this pull request

Jun 7, 2022

Consider supporting emscripten/webassembly as a build target #84461

Closed

Reviewers

brettcannon

Awaiting requested review from brettcannon

1 more reviewer

tiran

tiran left review comments

Reviewers whose approvals may not affect merge requirements

Uh oh!

There was an error while loading. Please reload this page.

Assignees

No one assigned

Labels

awaiting review skip news

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.

You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.

Conversation

emmatyping commented Jan 9, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emmatyping commented Jan 9, 2022

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

emmatyping commented Jan 12, 2022

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

emmatyping commented Jan 9, 2022 •

edited

Loading

Uh oh!

There was an error while loading. Please reload this page.