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

gh-95205: Improve wasm README #95206

Merged
merged 9 commits into from
Jul 25, 2022
Merged

gh-95205: Improve wasm README #95206

merged 9 commits into from
Jul 25, 2022

Conversation

erlend-aasland
Copy link
Contributor

@erlend-aasland erlend-aasland commented Jul 24, 2022 •
edited by bedevere-bot
Loading

@erlend-aasland
Copy link
Contributor Author

erlend-aasland commented Jul 24, 2022 •
edited
Loading

cc. @smontanaro: does this make things clearer, or should we make things more explicit?

erlend-aasland
erlend-aasland commented Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland commented Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@erlend-aasland erlend-aasland mentioned this pull request Jul 24, 2022
Closed
@smontanaro
Copy link
Contributor

LGTM. I think the -C flag should go as well.

@erlend-aasland
Copy link
Contributor Author

LGTM. I think the -C flag should go as well.

Thanks for reviewing. I've added the removal of -C as a suggestion; waiting for Christian to chime in before I land this.

@smontanaro
Copy link
Contributor

Note that there are missing "cpython" references in the "Cross compile to wasm32-emscripten for browser" section as well.

Finally (I think)... For Docker naifs it should be made explicit that when exiting and then reentering the docker world, previous work will be lost. If possible, it might well be worth showing users how to use Christian's Docker image as the base for creating a build environment which is preserved across runs. If I'm not making myself clear, let me know and I'll try to expand a bit.

@erlend-aasland
Copy link
Contributor Author

PTAL.

If possible, it might well be worth showing users how to use Christian's Docker image as the base for creating a build environment which is preserved across runs.

I'm not sure how explicit we should be regarding this. Let me know if my last update was too vague regarding this.

erlend-aasland
erlend-aasland commented Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland commented Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@tiran
Copy link
Member

tiran commented Jul 24, 2022

Leave the config.cache in the instructions. It is a massive time safer for everybody that wants to hack on WASM. emconfigure configure takes several minutes even on a fast machine.

tiran
tiran reviewed Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Leave the config.cache in the instructions. It is a massive time safer for everybody that wants to hack on WASM. emconfigure configure takes several minutes even on a fast machine.

Sure! I'll revert the last commit.

tiran
tiran requested changes Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
Tools/wasm/README.md Outdated Show resolved Hide resolved
@bedevere-bot
Copy link

When you're done making the requested changes, leave the comment: I have made the requested changes; please review again.

@erlend-aasland
Copy link
Contributor Author

Thanks, Christian. Specifying working dir is a better approach.

tiran
tiran requested changes Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
tiran
tiran reviewed Jul 24, 2022
View reviewed changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

Thanks for all your help, Christian!

@erlend-aasland
Copy link
Contributor Author

If you're fine with the last change, I'd like to land this, @tiran.

@tiran tiran added the needs backport to 3.11 only security fixes label Jul 25, 2022
@erlend-aasland erlend-aasland merged commit 310f948 into python:main Jul 25, 2022
@miss-islington
Copy link
Contributor

Thanks @erlend-aasland for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11.
🐍🍒⛏🤖

@erlend-aasland erlend-aasland deleted the doc-wasm-improvements branch July 25, 2022 07:42
@bedevere-bot
Copy link

GH-95230 is a backport of this pull request to the 3.11 branch.

@bedevere-bot bedevere-bot removed the needs backport to 3.11 only security fixes label Jul 25, 2022
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jul 25, 2022
@miss-islington
pythongh-95205: Improve wasm README (pythonGH-95206)
e96fe3a 
Co-authored-by: Christian Heimes <christian@python.org>
(cherry picked from commit 310f948)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
miss-islington added a commit that referenced this pull request Jul 25, 2022
@miss-islington
gh-95205: Improve wasm README (GH-95206)
df95ad3 
Co-authored-by: Christian Heimes <christian@python.org>
(cherry picked from commit 310f948)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@tiran tiran tiran approved these changes

Assignees
No one assigned
Labels
skip news
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@erlend-aasland @smontanaro @tiran @bedevere-bot @miss-islington