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

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
+6 −2

Conversation

erlend-aasland
Copy link
Contributor

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

@erlend-aasland erlend-aasland requested a review from tiran Jul 24, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jul 24, 2022 •
edited

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

erlend-aasland
erlend-aasland reviewed Jul 24, 2022
View changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland reviewed Jul 24, 2022
View 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

@smontanaro smontanaro commented Jul 24, 2022

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

@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jul 24, 2022

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

@smontanaro smontanaro commented Jul 24, 2022

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

@erlend-aasland erlend-aasland commented Jul 24, 2022

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 reviewed Jul 24, 2022
View changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
erlend-aasland
erlend-aasland reviewed Jul 24, 2022
View changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@tiran
Copy link
Member

@tiran 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 changes
Tools/wasm/README.md Outdated Show resolved Hide resolved
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland 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.

Sure! I'll revert the last commit.

tiran
tiran reviewed Jul 24, 2022
View changes
Tools/wasm/README.md Show resolved Hide resolved
tiran
tiran requested changes Jul 24, 2022
View 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

@bedevere-bot bedevere-bot commented Jul 24, 2022

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

@erlend-aasland erlend-aasland commented Jul 24, 2022

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

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

@erlend-aasland erlend-aasland commented Jul 24, 2022

Thanks for all your help, Christian!

@erlend-aasland erlend-aasland requested a review from tiran Jul 24, 2022
@erlend-aasland
Copy link
Contributor Author

@erlend-aasland erlend-aasland commented Jul 24, 2022

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

tiran
tiran approved these changes Jul 25, 2022
View changes
@erlend-aasland erlend-aasland merged commit 310f948 into python:main Jul 25, 2022
12 checks passed
@miss-islington
Copy link
Contributor

@miss-islington miss-islington commented Jul 25, 2022

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 Jul 25, 2022
@bedevere-bot
Copy link

@bedevere-bot bedevere-bot commented Jul 25, 2022

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

miss-islington pushed a commit to miss-islington/cpython that referenced this issue Jul 25, 2022
@erlend-aasland @miss-islington
pythongh-95205: Improve wasm README (pythonGH-95206)
e96fe3a 
Co-authored-by: Christian Heimes <christian@python.org>
(cherry picked from commit 310f94871a923f6cf3dc9259e732ce2376578b26)

Co-authored-by: Erlend Egeberg Aasland <erlend.aasland@innova.no>
miss-islington added a commit that referenced this issue Jul 25, 2022
@miss-islington @erlend-aasland
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

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

Successfully merging this pull request may close these issues.

None yet
5 participants
@erlend-aasland @smontanaro @tiran @bedevere-bot @miss-islington