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.

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

gh-126187 Add emscripten.py script to automate emscripten build #126190

Merged
merged 6 commits into from
Nov 9, 2024

Conversation

hoodmane
Copy link
Contributor

@hoodmane hoodmane commented Oct 30, 2024

This is modeled heavily on Tools/wasm/wasi.py. I tested it manually, hopefully we can soon use it as the basis for an Emscripten build bot.

There are a few hacks to work around problems. I prioritized adding a script that works as soon as possible without changing other files over making it non-hacky. I will clean it up in followup.

cc @freakboy3742.

@brettcannon
Copy link
Member

@freakboy3742 can you add yourself to CODEOWNERS for Tools/wasm?

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

Just tested this locally and it WFM [tm].

We should probably also update the instructions in Tools/wasm/README.md, which still point to the old script for building with emscripten.

@bedevere-app
Copy link

bedevere-app bot commented Oct 30, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@freakboy3742
Copy link
Contributor

@freakboy3742 can you add yourself to CODEOWNERS for Tools/wasm?

Done in #126210.

Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

The script itself looks great; a couple of minor formatting things that are likely the result of automated tooling.

As noted in another review, this needs at least some documentation for how to use it. The Tools/wasm/README.md contains a big section that would now seem to be largely redundant; so I'm inclined to suggest making that cleanup (or, at least, starting that cleanup) part of this PR.

The other question (which is a "how can we make @brettcannon's life easier" question). Do we want to - or even, can we - migrate the Emscripten-specific stuff into the Tools/wasm/emscripten folder? AIUI, the complicating factor here is that there is enough WASI tooling that is dependent on the Tools/wasm location, so moving that stuff to Tools/wasm/wasi isn't an option - so maybe a depth-based reorg isn't possible and keeping everything in Tools/wasm is fine. Any thoughts, @brettcannon?

Tools/wasm/emscripten.py Outdated Show resolved Hide resolved
Tools/wasm/emscripten.py Outdated Show resolved Hide resolved
@hoodmane
Copy link
Contributor Author

Do we want to - or even, can we - migrate the Emscripten-specific stuff into the Tools/wasm/emscripten folder?

I believe we can. My thought was to put emscripten.py in Tools/wasm/emscripten and try to move everything else Emscripten-specific into the folder. Open to other approaches.

@hoodmane hoodmane force-pushed the emscripten-py-tool branch 2 times, most recently from 51de036 to ae6f85b Compare October 31, 2024 09:55
@brettcannon
Copy link
Member

the complicating factor here is that there is enough WASI tooling that is dependent on the Tools/wasm location, so moving that stuff to Tools/wasm/wasi isn't an option

I've been thinking about it and if we are okay leaving Tools/wasm/wasi.py there for a while (like 5 releases), we can make a Tools/wasm/wasi/ directory and I can change wasi.py to call into the code in that directory (where I would probably drop a __main__.py into so it can be executed). Then I can have Tools/wasm/wasi.py raise a warning to direct people to update their stuff.

Does that sound like a reasonable plan?

@hoodmane
Copy link
Contributor Author

So in that case, I should put all new Emscripten files including this one under the emscripten subfolder?

If there's a __main__.py file, does that mean you expect people to invoke it like python -m Tools.wasm.wasi? If you are in the directory, python -m . doesn't work, so you would have to say python __main__.py? Unless I'm misunderstanding, I'd say that python Tools/wasm/wasi.py and having the helper files in the wasi subdirectory makes more sense to me personally. But if there is a way to set it up so that the cli usage isn't too weird it sounds fine.

@freakboy3742
Copy link
Contributor

So in that case, I should put all new Emscripten files including this one under the emscripten subfolder?

If there's a __main__.py file, does that mean you expect people to invoke it like python -m Tools.wasm.wasi? If you are in the directory, python -m . doesn't work, so you would have to say python __main__.py? Unless I'm misunderstanding, I'd say that python Tools/wasm/wasi.py and having the helper files in the wasi subdirectory makes more sense to me personally. But if there is a way to set it up so that the cli usage isn't too weird it sounds fine.

Whether the entry point is spelled python -m tools.wasm.wasi, python Tools/wasm/wasi.py, or something else entirely bother me. It's a script that will be used by such a small group of people that as long as it's documented, and the end-user ergonomics don't completely suck, I'm not sure it matters much.

My primary concern is getting separation between WASI- and Emscripten-specific code so that we can have confidence that a change to one target isn't going to impact the other (and, from a basic operational perspective, that CODEOWNERS notifications are accurately addressed).

@freakboy3742
Copy link
Contributor

...and realising that I didn't actually answer the specific question - yes, I would argue for putting this script into an emscripten subfolder.

@brettcannon
Copy link
Member

If there's a __main__.py file, does that mean you expect people to invoke it like python -m Tools.wasm.wasi?

Nope, I expect python Tools/wasm/wasi (you can point Python at a directory that has a __main__.py).

This is modeled heavily on `Tools/wasm/wasi.py`. I tested it manually, hopefully
we can soon use it as the basis for an Emscripten build bot.

There are a few hacks to work around problems. I prioritized adding a script
that works as soon as possible without changing other files over making it
non-hacky. I will clean it up in followup.
@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 7, 2024

I updated the readme to reflect the changes. There's more readme updates I want to make but it's enough mixed up with changes I've left to future PRs that I think it's better to include with that later work.

Copy link
Contributor

@mdboom mdboom left a comment

Choose a reason for hiding this comment

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

Thanks for the README changes. I see that this also closes #126201 now, which is great.

Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 7, 2024

Thanks for the grammar fixes @mdboom.

Co-authored-by: Michael Droettboom <[email protected]>
Copy link
Contributor

@freakboy3742 freakboy3742 left a comment

Choose a reason for hiding this comment

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

One Q about the docs, and one bug caused by the directory change.

Tools/wasm/emscripten/__main__.py Outdated Show resolved Hide resolved
```shell
git clone https://github.com/emscripten-core/emsdk.git --depth 1
./emsdk/emsdk install 3.1.68
./emsdk/emsdk activate 3.1.68
Copy link
Contributor

Choose a reason for hiding this comment

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

Any reason to nominate 3.1.68 specifically, rather than latest? I'm a little wary of putting a "magic number" into documentation unless that number is a minimum (like 3.1.42 above) or a pinned version of some significance.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay, I'll put latest for now and when we add actual CI I'll switch it to the specific version used in CI.

Copy link
Contributor

Choose a reason for hiding this comment

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

Out of interest - how stable/backwards compatible is "latest"? Is the emscripten version analogous to the manylinux tag/macOS dev version for compatibility purposes? Or is this a situation where we'll pick a specific emscripten version for all versions of a Python release (i.e., all 3.14.x releases are Emscripten 3.1.68)?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If we care about abi compatibility the safe way to go is to fix a version.

@bedevere-app
Copy link

bedevere-app bot commented Nov 8, 2024

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 8, 2024

Okay I have made the requested changes; please review again. Thanks for the review @freakboy3742.

@bedevere-app
Copy link

bedevere-app bot commented Nov 8, 2024

Thanks for making the requested changes!

@freakboy3742, @mdboom: please review the changes made to this pull request.

@hoodmane
Copy link
Contributor Author

hoodmane commented Nov 8, 2024

Huh interesting that the bot rerequested review from @mdboom even though his review status was already approved.

@freakboy3742 freakboy3742 merged commit f8276bf into python:main Nov 9, 2024
49 of 52 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants