--skip-npm flag to Skip putting emsdk's npm on PATH #1141

shlomif · 2022-12-01T11:47:55Z

See: * https://en.wikipedia.org/wiki/Code_refactoring * https://www.refactoring.com/ * https://www.joelonsoftware.com/2002/01/23/rub-a-dub-dub/ Some small optimisations may have slipped in as well.

sbc100

I don't know if this patch aligns with the approch that we discuessed in the bug, but if we do take this approach I think you would just want to modify get_required_path. We still want to install and activate our own version of node, we just don't want to add it to the users path, right?

emsdk.py

sbc100 · 2022-12-01T16:58:19Z

emsdk.py

    # If this tool/sdk depends on other tools, require that all dependencies are
    # installed for this tool to count as being installed.
    if hasattr(self, 'uses'):
      for tool_name in self.uses:
+        if skip_npm:
+          if tool_name.startswith('node') or tool_name.startswith('npm'):


No need to the second part of this condition.

How about just if skip_npm and ool_name.startswith('node'):

sbc100 · 2022-12-01T16:58:40Z

emsdk.py

@@ -1870,10 +1875,14 @@ def update_installed_version(self):
    return None

  def is_installed(self, skip_version_check=False):
+    global skip_npm


No need to global here (its only needed if you want to update a global).

sbc100 · 2022-12-01T16:58:47Z

emsdk.py

@@ -1398,6 +1400,9 @@ def find_latest_installed_tool(name):

 # npm install in Emscripten root directory
 def emscripten_npm_install(tool, directory):
+  global skip_npm


No need to global here (its only needed if you want to update a global).

sbc100 · 2022-12-01T16:59:37Z

emsdk.py

@@ -1398,6 +1400,9 @@ def find_latest_installed_tool(name):

 # npm install in Emscripten root directory
 def emscripten_npm_install(tool, directory):
+  global skip_npm
+  if skip_npm:
+    return True


If the goal is just to avoid putting the emscripten version of not in the path, you don't need to return early here. You will want to run npm install after installing emscripten.

Per the comments on the pullreq

shlomif · 2022-12-06T08:00:13Z

@sbc100 : thx. i fixed some low-hanging fruit

…-core#1145)

Fixes emscripten-core#1020

fastcomp can only be install using explicit versions names so this name doesn't work.

Folks that want to work with fastcomp will now need to use an older checkout of emsdk.

This name existed to distinguish the SDK from fastcomp, but as of emscripten-core#1165, we no longer support fastcomp.

These function already returns a string.

* Add support for Visual Studio 2022 and migrate to using cmake --build when building on Windows. Leverage the VS2019 MSBuild 'Multi-ToolTask' feature CL_MPCount to saturate project builds properly to 100% CPU usage so building LLVM builds different cpp files in parallel. Clean up some code duplication around Visual Studio support. * flake * Work around Linux bot not having 'cmake --build . -j' flag.

…#1179)

…22, upstream LLVM has required VS2019 or VS2022 to build. So it has not been possible to build emsdk from source with VS2017 for a year. llvm/llvm-project@058c5df (emscripten-core#1178)

We have an existing `version_key` helper function for sorting versions. It also does a better job, producing output like: ``` All recent (non-legacy) installable versions are: 3.1.31 3.1.31-asserts 3.1.30 3.1.30-asserts 3.1.29 3.1.29-asserts ``` Rather than: ``` All recent (non-legacy) installable versions are: 3.1.31 3.1.30 3.1.29 3.1.28 3.1.27 ``` (with -assert versions listed after 3.1.0)

…1186)

…pten-core#1187)

- Remove reference to `~/.emscripten`. We no longer use the home directory to store config information by default, either in emscripten or in emsdk. - Remove some references to `The Emscripten Command Prompt`. While I suppose this is referring to the windows-only `emcmdprompt.bat`, and I suppose it means "any shell where `activate` has been run", I think its more clear to simply avoid using the term.

…ten-core#1189) If the user already has a version of node in their PATH don't clobber it. This doesn't effect emscripten since the version of node we use there is controlled via the config file, not via PATH. Part of fix for emscripten-core#705.

…1222) This runner is faster and more efficient. Also factor the mac configuration into an executor.

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

The fastcomp backend was removed in Emscripten v2.0.0.

This allows us to use the native ARM64 version on MacOS. Also update the test scripts to work on ARM64 mac, and skip tests that aren't relevant.

This seems like more commonly known/used name for the architecture these days.

…-core#1249) See emscripten-core#547

…ripten-core#1250) The first time around `node` was being correctly added to the PATH, but the second time around this code was observing the emsdk copy of node in the PATH and assuming it could be skipped. Fixes: emscripten-core#1240

In favor of `/etc/bash.bashrc`, which is only read for interactive shells.

…core#1255) Having used this script for a while now I'm not sure there is any point if leaving this last step as a manual push.

…ripten-core#1257)

…m. (emscripten-core#1262) * wasm_cc_binary: Specify a default OS. Allow users to override platform. Problem: proxy-wasm/proxy-wasm-cpp-sdk#157 (comment) This is solving the problem in two different ways. Please let me know your thoughts about both approaches, as either will work. Signed-off-by: Martijn Stevenson <mstevenson@google.com> * Rework platform selection to trigger os:wasi off standalone attr Signed-off-by: Martijn Stevenson <mstevenson@google.com> --------- Signed-off-by: Martijn Stevenson <mstevenson@google.com>

Mark the build files as using the starlark language so they have the correct syntax highlighting.

Also update emscripten include dir to v18, and change 17 to wildcard in emscripten deps

…from the py2 linter (emscripten-core#1272) Flake8's INI config file format requires commas after each line. Because our file didn't have them, the exclude list wasn't set up correctly, and the --extend-exclude flag wasn't working. This PR fixes the .flake8 file. Also, update flake8 to the latest version available (because version 3.8 is required to get the --extend-exclude flag) and use the flag to exclude the files in the scripts/ directory from the python2 linter (since the scripts are python3).

…re#1271) This script is (IMO) more readable, but the real reason for this change is that it raises an error message when the binary package fails to download. (The shell script silently generated a bogus hash instead, because the shell's `set -e` builtin does not affect commands executing inside a $() context. It seemed just as easy to rewrite the script in Python as to fix that. This change also updates some outdated filename references.

Fixes emscripten-core#1273. This was broken by emscripten-core#1045 with the comment "* all_files not needed?" They were needed.

This is a bit of a hack but I can't think of another way to do it. Basically when downloading SDKs, we first try the new `.xz` extension. If that fails, we fall back to the old `.tbz2`. Both these first two download attempts we run in "silent" mode. If both of them fail we re-run the original request in non-silent mode so that the error message will always contain the original `.xz` extension. See emscripten-core#1235

sbc100 · 2023-11-17T21:01:27Z

I think this can be closed now that #1189 has landed emsdk will no longer add its node to the PATH if you already have one .

shlomif added 3 commits December 1, 2022 13:42

skip-npm

156dccb

emscripten-core#1048

cleanup

9cc8ed7

Refactoring / code cleanup.

787da0b

See: * https://en.wikipedia.org/wiki/Code_refactoring * https://www.refactoring.com/ * https://www.joelonsoftware.com/2002/01/23/rub-a-dub-dub/ Some small optimisations may have slipped in as well.

sbc100 reviewed Dec 1, 2022

View reviewed changes

Rename to skip_node and cleanup.

e6d333b

Per the comments on the pullreq

shlomif and others added 24 commits December 23, 2022 15:23

refactor

bac1329

Add standalone_wasm feature to bazel emscripten_toolchain (emscripten…

65bc87d

…-core#1145)

3.1.28 (emscripten-core#1149)

7d76e9e

Upgrade to rules_nodejs 5.8.0 (emscripten-core#1150)

0d1238e

Fixes emscripten-core#1020

3.1.29 (emscripten-core#1160)

f999f4d

Pin Windows CI to Bazel 5.4.0 (emscripten-core#1163)

f3bc9f2

Remove reference to fastcomp-latest. NFC (emscripten-core#1164)

346e233

fastcomp can only be install using explicit versions names so this name doesn't work.

Remove fastcomp SDK and fastcomp build rules. NFC (emscripten-core#1165)

c299f67

Folks that want to work with fastcomp will now need to use an older checkout of emsdk.

3.1.30 (emscripten-core#1167)

c25efd6

Remove -upstream component of SDK names (emscripten-core#1166)

5991457

This name existed to distinguish the SDK from fastcomp, but as of emscripten-core#1165, we no longer support fastcomp.

Remove uneeded str. NFC (emscripten-core#1170)

8c09461

These function already returns a string.

Update README.md after emscripten-core#1166. NFC (emscripten-core#1172)

ba3a3c8

3.1.31 (emscripten-core#1176)

3009923

Fix args ordering in LLVM build from previous commit (emscripten-core…

22f2521

…#1179)

Remove support for building with Visual Studio 2017. Since Jan 29, 20…

f90d188

…22, upstream LLVM has required VS2019 or VS2022 to build. So it has not been possible to build emsdk from source with VS2017 for a year. llvm/llvm-project@058c5df (emscripten-core#1178)

Add Windows Arm64 support for compiling from source (emscripten-core#…

c98ba98

…1186)

Remove unused build_target_platform parameter from make_build (emscri…

7c66ba7

…pten-core#1187)

3.1.32 (emscripten-core#1188)

31c28db

Add missing binaries for bazel (emscripten-core#1082)

7a71a71

3.1.33 (emscripten-core#1200)

adc2846

dschuff and others added 29 commits September 29, 2023 21:18

3.1.40 (emscripten-core#1221)

bda6e3d

Update mac builders to use Circle's gen2 mac runner (emscripten-core#…

b5fb454

…1222) This runner is faster and more efficient. Also factor the mac configuration into an executor.

Add security policy (emscripten-core#1224)

d59bea6

Signed-off-by: Pedro Kaj Kjellerup Nacht <pnacht@google.com>

Docker: remove remnant of fastcomp (emscripten-core#1226)

41a6db6

The fastcomp backend was removed in Emscripten v2.0.0.

Docker: remove redundant EMSDK_NODE env (emscripten-core#1225)

e2f87da

3.1.41 (emscripten-core#1230)

fcbbb5c

Update node, 15.14.0 -> 16.20.0 (emscripten-core#1232)

d094326

This allows us to use the native ARM64 version on MacOS. Also update the test scripts to work on ARM64 mac, and skip tests that aren't relevant.

3.1.42 (emscripten-core#1238)

5690fde

Prefer arm64 over aarch64 internally. NFC (emscripten-core#1246)

6fb20c4

This seems like more commonly known/used name for the architecture these days.

Add some basic testing of arm64 linux (emscripten-core#1247)

6dbcaff

Add a warning when trying to install linux-arm64 binaries (emscripten…

ab10d25

…-core#1249) See emscripten-core#547

Fix a typo (emscripten-core#1251)

2933da6

Docker: use a entrypoint script (emscripten-core#1227)

42b482c

In favor of `/etc/bash.bashrc`, which is only read for interactive shells.

3.1.43 (emscripten-core#1254)

a971fe4

Have create_release.py push the new branch automatically (emscripten-…

a2ca455

…core#1255) Having used this script for a while now I'm not sure there is any point if leaving this last step as a manual push.

Update linux arm64 to 3.1.41 (emscripten-core#1256)

c3870d4

Rename zipfile to archive or download as appropriate. NFC (emsc…

8c1d600

…ripten-core#1257)

Update linux arm64 to 3.1.43 (emscripten-core#1258)

a760086

3.1.44 (emscripten-core#1260)

b25d87e

Add starlark highlighting to Bazel readme (emscripten-core#1264)

d27c69b

Mark the build files as using the starlark language so they have the correct syntax highlighting.

3.1.45 (emscripten-core#1269)

d021b3d

Also update emscripten include dir to v18, and change 17 to wildcard in emscripten deps

Update linux arm64 build. NFC (emscripten-core#1270)

dd94117

[bazel] populate all_files (emscripten-core#1274)

0b10952

Fixes emscripten-core#1273. This was broken by emscripten-core#1045 with the comment "* all_files not needed?" They were needed.

3.1.46 (emscripten-core#1279)

f9f0ba3

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

--skip-npm flag to Skip putting emsdk's npm on PATH #1141

--skip-npm flag to Skip putting emsdk's npm on PATH #1141

shlomif commented Dec 1, 2022 •

edited by sbc100

sbc100 left a comment

sbc100 Dec 1, 2022

sbc100 Dec 1, 2022

sbc100 Dec 1, 2022

sbc100 Dec 1, 2022

shlomif commented Dec 6, 2022

sbc100 commented Nov 17, 2023

--skip-npm flag to Skip putting emsdk's npm on PATH #1141

Are you sure you want to change the base?

--skip-npm flag to Skip putting emsdk's npm on PATH #1141

Conversation

shlomif commented Dec 1, 2022 • edited by sbc100

sbc100 left a comment

Choose a reason for hiding this comment

sbc100 Dec 1, 2022

Choose a reason for hiding this comment

sbc100 Dec 1, 2022

Choose a reason for hiding this comment

sbc100 Dec 1, 2022

Choose a reason for hiding this comment

sbc100 Dec 1, 2022

Choose a reason for hiding this comment

shlomif commented Dec 6, 2022

sbc100 commented Nov 17, 2023

shlomif commented Dec 1, 2022 •

edited by sbc100