Skip to content

Support emsdk --embedded mode #10935

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
Merged
merged 4 commits into from
Apr 17, 2020
Merged

Support emsdk --embedded mode #10935

merged 4 commits into from
Apr 17, 2020

Conversation

sbc100
Copy link
Collaborator

@sbc100 sbc100 commented Apr 16, 2020 •
edited
Loading

This change allows emsdk --embdded mode to work out of the box
without running emsdk_env.sh.

This paves makes it easier to justify using embedded mode by default:
emscripten-core/emsdk#472

This is part of a larger plan to avoid using the $HOME directory to
store anything at all: #9543

@sbc100 sbc100 requested a review from kripken April 16, 2020 00:09
waywardmonkeys
waywardmonkeys reviewed Apr 16, 2020
View reviewed changes
kripken
kripken reviewed Apr 16, 2020
View reviewed changes
tools/shared.py Outdated
@@ -265,6 +265,12 @@ def generate_config(path, first_time=False):
# 4. Fall back users home directory (~/.emscripten).

embedded_config = path_from_root('.emscripten')
# For compatability with emsdk --embedded mode also look two levels up. This
# could be removed in emsdk was use the emscripten directory itself for the
Copy link
Member

Choose a reason for hiding this comment

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

I don't follow this statement - should in be if, and use be using?

But I'm not sure I get it. When are embedded_config and emsdk_embedded_config different? Clarifying that would be useful here I think.

Copy link
Collaborator Author

@sbc100 sbc100 Apr 16, 2020 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks, fixed the grammar there.

The issue is that emsdk's --embedded mode currently always puts the .emscripton config file in the root of the emsdk, which is two levels up from the emscripten directory. I'm working on trying to change that but it seems a little complicated.

emscripten already has support for an embedded config, but only its own directory.

This change is only really needs for users who don't run emsdk_env.sh and just run emcc directly.

Most users are expected to run emsdk_env.sh to use the SDK, but it turns out the if you just run /path/to/emscripten/emcc without running emsdk_env.sh today it often just works because emscripten knowns how to find its config and everything flows from there. Without this change if you use emsdk --embedded and then run /path/to/emcc without having first run emsdk_env.sh then emscripten will not find any config file at all and default back to its method of create new config file with default settings in $HOME.

This change means that emsdk users won't see this default bahaviour becuse them will look in the correct plance for the embedded config file, even if EM_CONFIG is not set (which it normally is when you run emsdk_env.sh).

Sorry for the long explanation, its kind of annoyingly complicated.

sbc100 added 2 commits April 16, 2020 10:36
@sbc100
Support emsdk --embedded mode
de01380 
This change allows emsdk --embdded mode to work out of the box
without running emsdk_env.sh.

This paves makes it easier to justify using embedded mode by default:
  emscripten-core/emsdk#472

This is part of a larger plan to avoid using the $HOME directory to
store anything at all: #9543
@sbc100
feedback
bb27346
@sbc100 sbc100 force-pushed the support_emsdk_embedded branch from 3eb672b to bb27346 Compare April 16, 2020 17:48
@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 16, 2020

tldr; This change is the minimize the impact of switch to --embedded by default in the emsdk.

kripken
kripken reviewed Apr 16, 2020
View reviewed changes
tools/shared.py Outdated
@@ -265,6 +265,12 @@ def generate_config(path, first_time=False):
# 4. Fall back users home directory (~/.emscripten).

embedded_config = path_from_root('.emscripten')
# For compatibility with emsdk --embedded mode also look two levels up. This
Copy link
Member

Choose a reason for hiding this comment

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

maybe mention why two levels up? That is, that when using the emsdk, the structure would look like

emsdk/
  emscripten/

But that picture must be wrong since you say two levels and not one..?

@kripken
Copy link
Member

kripken commented Apr 16, 2020

The tl;dr sounds good! Sorry I don't get the technical details yet.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 16, 2020

Added more comments that should make it more obvious why we need this.

@kripken
Copy link
Member

kripken commented Apr 16, 2020 •
edited by sbc100
Loading

I'm sorry, I still don't get it  why 2 levels up? Is it not this:

emsdk/
  emscripten/

Where is the extra directory? I really think it would be good to add a little drawing like that into the comment, it would have helped me get this a lot sooner.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 16, 2020

Also not that this change is more a convenience for emsdk users that forget to run ./emsdk_env.sh.

emsdk users that correctly set thier environment will not be effected by this.

It also only effects users of --embedded which hopefully soon will be all users :)

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 16, 2020

The places that emscripten lives with the emsdk look like this:

emsdk/
  fastcomp/
    emscripten/

emsdk/
  upstream/
    emscripten/

Or if building from source they look like:

emsdk/
  1.38.30/
    emscripten/

It never lives directly in emsdk/emscripten.

kripken
kripken approved these changes Apr 16, 2020
View reviewed changes
Copy link
Member

@kripken kripken left a comment

Choose a reason for hiding this comment

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

I see now, thanks!

lgtm with an explanation for the 2 levels in a comment. I think something like your last comment would be great.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 16, 2020

I did add a lot of comments to the code already. Did you look at the latest version? Do you think it needs more?

kripken
kripken approved these changes Apr 17, 2020
View reviewed changes
tools/shared.py Outdated
# 3. If emscripten-local .emscripten file is found, use that
# 4. Fall back users home directory (~/.emscripten).
# 3. Local .emscripten file, if found
# 4. Local .emscripten file, as written by emsdk --embedded (two levels up), if found.
Copy link
Member

Choose a reason for hiding this comment

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

It says "two levels" here, but without a little graph like in that comment of yours, it's very unclear I think.

likewise, on line 269 it also says "two levels" but doesn't explain.

If I'm being unclear here, I apologize, and feel free to land this - I'll open a followup PR.

@sbc100
Copy link
Collaborator Author

sbc100 commented Apr 17, 2020

I included some exampels.

@kripken
Copy link
Member

kripken commented Apr 17, 2020

Thanks!

@sbc100 sbc100 merged commit a69a3b1 into master Apr 17, 2020
@sbc100 sbc100 deleted the support_emsdk_embedded branch April 17, 2020 02:40
@sbc100 sbc100 mentioned this pull request Apr 18, 2020
Merged
@sbc100 sbc100 mentioned this pull request Sep 20, 2022
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@waywardmonkeys waywardmonkeys waywardmonkeys left review comments

@kripken kripken kripken approved these changes

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

Successfully merging this pull request may close these issues.
3 participants
@sbc100 @kripken @waywardmonkeys