Add pkgsCross.emscripten, replacing hand-crafted emscriptenPackages #217428

AtnNn · 2023-02-21T04:37:58Z

Description of changes

Added a pre-built Emscripten SYSTEM cache to the emscripten package
Added emscriptenFull which additionally contains Emscripten PORTS
Added emscriptenNoCache which requires setting EM_CACHE to use
Add wrapped emscriptenCC and emscriptenBintools
Added Emscripten to lib/systems and pkgsCross
Moved the xmlmirror package from inside top-level/emscripten-packages.nix to tools/text/xml
Removed custom emscriptenPackages.json_c and emscriptenPackages.zlib
Removed buildEmscriptenPackage
Added some pkgsCross.emscripten packages to release-cross.nix

Possibly fixes #28929 #141246 #136396. Improves the fix for #139943.

Tested with

$ nix-build --show-trace -E 'with import ./pkgs/top-level/release-cross.nix {}; builtins.mapAttrs (k: v: v."x86_64-linux") emscripten'
$ node result-3/bin/hello
Hello, world!

Things done

AtnNn · 2023-02-21T04:53:57Z

What do you think about this approach? @qknight @matthewbauer @RaitoBezarius

RaitoBezarius · 2023-02-23T04:54:13Z

What do you think about this approach? @qknight @matthewbauer @RaitoBezarius

Interesting changes, thank you for doing this. I am not too expert in the stdenv area, so I would rather defer to people working on that, cc @alyssais @Ericson2314

I will need more time to review though and test thoroughly, but it seems to be a PR in the right direction.

alyssais

This is a very promising direction. Thank you for exploring it! Since it's still early, I've focused my review only on the stdenv stuff, not any individual package modifications.

The biggest threat I see is the split binaries, so some more info on those would be great.

lib/systems/examples.nix

lib/systems/inspect.nix

pkgs/development/compilers/emscripten/wrapped.nix

pkgs/stdenv/generic/make-derivation.nix

pkgs/tools/misc/figlet/default.nix

pkgs/top-level/all-packages.nix

AtnNn · 2023-03-25T14:46:47Z

Thanks @alyssais. I'm going to update this branch to use Emscripten's -sSINGLE_FILE mode, which seems to solve the issues I had with split executables.

mohe2015 · 2023-05-12T20:13:44Z

Could you please fix the syntax errors in overlay.nix? And maybe update to latest nixpkgs?

RaitoBezarius · 2023-05-14T16:56:56Z

Let's get this after the branch-off :).

willcohen · 2023-05-19T18:56:53Z

Just as a note for a parallel emscripten change, #229718 (not merged as of this comment) would bump underlying emscripten from 3.1.24 to 3.1.39, which in turn requires LLVM 16 instead of 14.

lib/strings.nix

lib/systems/inspect.nix

SuperSandro2000 · 2023-05-22T17:57:45Z

lib/systems/parse.nix

@@ -294,6 +294,7 @@ rec {
    openbsd  = { execFormat = elf;     families = { inherit bsd; }; };
    solaris  = { execFormat = elf;     families = { }; };
    wasi     = { execFormat = wasm;    families = { }; };
+    emscripten     = { execFormat = wasm;    families = { }; };


Suggested change

emscripten = { execFormat = wasm; families = { }; };

emscripten = { execFormat = wasm; families = { }; };

Best would be to adjust the alignment of the others to match.

SuperSandro2000 · 2023-05-22T17:58:12Z

pkgs/build-support/cc-wrapper/default.nix

-, isGNU ? false, isClang ? cc.isClang or false, gnugrep ? null
+, isGNU ? false
+, isClang ? cc.isClang or false
+, isEmscripten ? cc.isEmscripten or false


This is not used

SuperSandro2000 · 2023-05-22T17:59:25Z

pkgs/stdenv/generic/make-derivation.nix

-          explicitFlags ++ lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) crossFlags;
+          explicitFlags
+          ++ lib.optionals (stdenv.hostPlatform != stdenv.buildPlatform) crossFlags
+          ++ (stdenv.extraCmakeFlags or []) ;


Suggested change

++ (stdenv.extraCmakeFlags or []) ;

++ (stdenv.extraCmakeFlags or []);

The parens aren't necessary either, I think.

I would prefer keeping the parens as it makes it more clear what is meant.

I would trust deadnix. If it says they are optional, I would remove them.

SuperSandro2000 · 2023-05-22T18:04:36Z

pkgs/stdenv/emscripten/overlay.nix

+    bullet = bullet;
+    bzip2 = bzip2;
+    giflib = giflib;
+    harfbuzz = harfbuzz;
+    icu = icu;
+    libjpeg = libjpeg;
+    libmodplug = libmodplug;
+    libpng = libpng;
+    mpg123 = mpg123;


Suggested change

bullet = bullet;

bzip2 = bzip2;

giflib = giflib;

harfbuzz = harfbuzz;

icu = icu;

libjpeg = libjpeg;

libmodplug = libmodplug;

libpng = libpng;

mpg123 = mpg123;

inherit bullet bzip2 giflib harfbuzz icu libjpeg libmodplug libpng mpg123 zlib;

SuperSandro2000 · 2023-05-22T18:04:43Z

pkgs/stdenv/emscripten/overlay.nix

+    libogg = ogg;
+    sqlite = sqlite3;
+    libvorbis = vorbis;
+    zlib = zlib;


Suggested change

zlib = zlib;

pkgs/development/libraries/gobject-introspection/default.nix

SuperSandro2000 · 2023-05-22T18:10:03Z

pkgs/development/compilers/emscripten/ports.nix

+    sha256 = "sha256-hzXN30IocklLty8daZSF5rHQJ4xHKLWzCQP7o0m5Z0s=";
+  };
+  libpng  = {
+    url = "https://github.com/libsdl-org/SDL/archive/release-2.24.0.zip";


At least this URL is duplicated. Not sure if it is worth to dedupe. Also can't we re-use the src's from our packages?

I've fixed the duplicate URLs. The src's from nixpkgs cannot be used here, the url in this file point to the exact sources that embuilder.py requires.

SuperSandro2000 · 2023-05-22T18:24:00Z

pkgs/development/compilers/emscripten/cc.nix

+  buildInputs = [ makeWrapper ];
+
+  unpackPhase = ''
+    src=$PWD


Can we use sourceRoot?

I just copied this from pkgs/build-support/bintools-wrapper/default.nix. How would I use srcRoot instead?

This is okay, the point is to forgo upacking (since there is no source), but src needs to be set to something. sourceRoot only controls the working directory which is irrelevant here.

pkgs/development/compilers/emscripten/cc.nix

pkgs/development/compilers/emscripten/cache.nix

pkgs/development/compilers/emscripten/bintools.nix

willcohen · 2023-05-26T14:18:06Z

Noting that #229718, which bumps underlying emscripten and llvm, has entered staging.

Artturin · 2023-06-26T19:50:36Z

lib/systems/default.nix

        emulatorAvailable = pkgs: (selectEmulator pkgs) != null;

-        emulator = pkgs:
+        emulatorNeeded = pkgs:


Unused and there's already canExecute

Artturin · 2023-07-08T21:21:07Z

pkgs/top-level/all-packages.nix

+  makeWrapper =
+    if stdenv.hostPlatform.isWindows || stdenv.hostPlatform.isEmscripten
+    then null


should throw or something

should break early

comment why it doesn't work on emscripten

can't be changed to makeBinaryWrapper globally on windows #120726 (comment)

My intent here is to allow packages to conditionally use makeWrapper if it is available by checking for makeWrapper == null. Changing this to a throw wouldn't work for that. Is there a better way to do that? Maybe something like lib.elem stdenv.hostPlatform makeWrapper.meta.platforms?

Indeed, the using expression should check whether something is available or not. lib.meta.availableOn should suffice for this, given that the compatibility is expressed in (makeWrapper's) meta.platforms.

Using null will also break makeWrapper in buildPackages for the affected package sets due to splicing depending on attribute sets.

sternenseemann · 2023-07-09T11:05:12Z

pkgs/development/compilers/emscripten/bintools.nix

+  passthru = {
+    libc_bin = "";
+    libc_dev = "";
+    libc_lib = "";


null is better here.

sternenseemann · 2023-07-09T11:08:34Z

pkgs/top-level/all-packages.nix

+  makeWrapper =
+    if stdenv.hostPlatform.isWindows || stdenv.hostPlatform.isEmscripten
+    then null


Indeed, the using expression should check whether something is available or not. lib.meta.availableOn should suffice for this, given that the compatibility is expressed in (makeWrapper's) meta.platforms.

Using null will also break makeWrapper in buildPackages for the affected package sets due to splicing depending on attribute sets.

sternenseemann · 2023-07-09T11:09:08Z

pkgs/top-level/all-packages.nix

@@ -7469,10 +7472,6 @@ with pkgs;

  zzuf = callPackage ../tools/security/zzuf { };

-  ### DEVELOPMENT / EMSCRIPTEN
-
-  buildEmscriptenPackage = callPackage ../development/em-modules/generic { };


This is a substantial, breaking change and should also be explained in the release notes for 23.11.

sternenseemann · 2023-07-09T11:09:52Z

doc/languages-frameworks/emscripten.section.md


-* nix
-* nixpkgs
+This mode is far more power full since this makes use of `nix` for


Suggested change

This mode is far more power full since this makes use of `nix` for

This mode is far more powerful since this makes use of `nix` for

Though this sentence doesn't really make sense: More powerful than what? Imperative Usage is only described later on…

sternenseemann · 2023-07-09T11:12:38Z

lib/systems/default.nix

        emulatorAvailable = pkgs: (selectEmulator pkgs) != null;

-        emulator = pkgs:
+        emulatorCommand = pkgs:


I dislike introducing yet another field here, how is this necessary?

The CMake variable CMAKE_CROSSCOMPILING_EMULATOR requires a semicolon-delimited command.

sternenseemann · 2023-07-09T11:17:13Z

pkgs/development/compilers/emscripten/cc.nix

+  buildInputs = [ makeWrapper ];
+
+  unpackPhase = ''
+    src=$PWD


This is okay, the point is to forgo upacking (since there is no source), but src needs to be set to something. sourceRoot only controls the working directory which is irrelevant here.

sternenseemann · 2023-07-09T11:19:55Z

pkgs/development/libraries/libxml2/default.nix

@@ -27,7 +27,9 @@ let
  # https://gitlab.gnome.org/GNOME/libxml2/-/commit/b706824b612adb2c8255819c9a55e78b52774a3c
  # This case is encountered "temporarily" during stdenv bootstrapping on darwin.
  # Beware that the old version has known security issues, so the final set shouldn't use it.
-  oldVer = python.pname == "python3-minimal";
+  oldVer =
+    (pythonSupport || stdenv.isDarwin) # testing for isDarwin only to avoid mass rebuild (stdenv contains libxml2)


Just target staging.

sternenseemann · 2023-07-09T11:28:34Z

pkgs/development/libraries/openssl/default.nix

@@ -172,7 +174,7 @@ let
      mkdir -p $bin
      mv $out/bin $bin/bin

-    '' + lib.optionalString (!stdenv.hostPlatform.isWindows)
+    '' + lib.optionalString (makeWrapper != null)


lib.meta.availableOn makeWrapper stdenv.hostPlatform would be a possibility or querying meta.broken of makeWrapper—or just expanding the conditional.

For better or worse, the convention in nixpkgs is that the expression using a package decides whether it can use it or not. This also has the sense that it prevents stuff from being silently unavailable which can alter configure time settings etc.

sternenseemann · 2023-07-09T11:29:44Z

pkgs/stdenv/generic/make-derivation.nix

@@ -380,7 +380,7 @@ else let
          "-DCMAKE_HOST_SYSTEM_PROCESSOR=${stdenv.buildPlatform.uname.processor}"
        ] ++ lib.optionals (stdenv.buildPlatform.uname.release != null) [
          "-DCMAKE_HOST_SYSTEM_VERSION=${stdenv.buildPlatform.uname.release}"
-        ]);
+        ]) ++ (stdenv.extraCmakeFlags or []);


I think this is better achieved with a setup hook that appends to cmakeFlags.

AtnNn · 2023-07-10T18:47:02Z

Thanks for the suggestions. I've:

removed escapeShellArgs' and emulatorCommand
used a setup hook for CMake flags
stopped avoiding rebuilds and target staging instead

Artturin · 2023-07-10T20:15:41Z

pkgs/build-support/setup-hooks/emscripten-cmake-hook.sh

+addEnvHooks "$hostOffset" addEmscriptenCMakeEnv
+
+


trailing newlines should be removed

Artturin · 2023-07-10T20:25:39Z

pkgs/development/compilers/emscripten/bintools.nix

+  targetPrefix = "${wasmArch}-emscripten-";
+  llvm = emscripten.llvmEnv;
+
+unwrapped = stdenv.mkDerivation {


wrong indentation, run git diff --name-only HEAD~ --diff-filter=A | grep "\.nix" | xargs nixpkgs-fmt to format added files only

Artturin · 2023-07-10T20:27:12Z

pkgs/development/compilers/emscripten/bintools.nix

+  unpackPhase = ''
+    src=$PWD
+  '';


Suggested change

unpackPhase = ''

src=$PWD

'';

dontUnpack = true;

same for the others

Artturin · 2023-07-10T20:28:54Z

pkgs/development/compilers/emscripten/bintools.nix

+    src=$PWD
+  '';
+
+  installPhase = ''


runHook's missing
also in other files

Artturin · 2023-07-10T20:29:42Z

pkgs/development/compilers/emscripten/bintools.nix

+    lnLLVM llvm-cxxfilt c++filt
+    lnLLVM llvm-addr2line addr2line
+
+    echo -n > $out/nix-support/libc-ldflags-before


why?, add a comment.

Artturin · 2023-07-10T20:35:39Z

pkgs/top-level/all-packages.nix

@@ -21135,6 +21138,7 @@ with pkgs;
    else if name == "nblibc" then targetPackages.netbsdCross.libc or netbsdCross.libc
    else if name == "wasilibc" then targetPackages.wasilibc or wasilibc
    else if name == "relibc" then targetPackages.relibc or relibc
+    else if stdenv.targetPlatform.isEmscripten then null


why is emscripten special from all the others, why is stdenv.targetPlatform.libc not null on emscripten

I'll remove this and set libc = null

Artturin · 2023-07-10T20:41:08Z

pkgs/shells/bash/5.nix

@@ -136,7 +136,7 @@ stdenv.mkDerivation rec {
    '';
    license = licenses.gpl3Plus;
    platforms = platforms.all;
-    maintainers = with maintainers; [ dtzWill ];


accidentally removed

Artturin · 2023-07-10T20:51:46Z

lib/tests/systems.nix

@@ -95,6 +95,7 @@ lib.runTests (
        # The functions should be derived from the data, so this is not a problem.
        canExecute = null;
        emulator = null;
+        emulatorCommand = null;


Suggested change

emulatorCommand = null;

leftover

Artturin · 2023-07-10T20:52:25Z

nixos/doc/manual/release-notes/rl-2311.section.md

@@ -125,3 +125,5 @@ The module update takes care of the new config syntax and the data itself (user
  ./common/auto-format-root-device.nix ];` When you use the systemd initrd, you
  can automatically format the root device by setting
  `virtualisation.fileSystems."/".autoFormat = true;`.
+
+- `emscriptenPackages` and `buildEmscriptenPackage` have been replaced with `pkgsCross.emscripten` and `pkgsCross.emscripten.stdenv.mkDerivation`.


don't add directly at the end but at a random location to prevent merge conflicts

willcohen · 2023-10-12T17:50:27Z

As a quick note I've discovered working through #260267 -- emscripten will now need access to emscripten's node_modules folder as well, so it can run acorn-optimizer.js during some of its passes.

allsey87 · 2024-04-23T14:11:07Z

I was experimenting a bit with this PR and considering to revive it and noticed that the following works:

nix-build '<nixpkgs>' -I nixpkgs=https://github.com/AtnNn/nixpkgs/archive/emscripten.tar.gz \
  -A pkgsCross.emscripten.hello

However, this does not work:

nix-build '<nixpkgs>' -I nixpkgs=https://github.com/AtnNn/nixpkgs/archive/emscripten.tar.gz \
  --arg crossSystem '{ config = "wasm32-unknown-emscripten"; }' \
  -A hello

Instead it fails with the error, "Unknown libc emscripten".

Is this expected?

alyssais requested changes Mar 22, 2023

View reviewed changes

alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 22, 2023

github-actions bot added 6.topic: python and removed 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS labels Mar 27, 2023

ofborg bot removed the 2.status: merge conflict This PR has merge conflicts with the target branch label Mar 27, 2023

willcohen mentioned this pull request May 22, 2023

emscriptenStdenv: document .emscriptencache #181363

Closed

13 tasks

SuperSandro2000 reviewed May 22, 2023

View reviewed changes

lib/strings.nix Outdated Show resolved Hide resolved

SuperSandro2000 reviewed May 22, 2023

View reviewed changes

lib/systems/inspect.nix Show resolved Hide resolved

SuperSandro2000 reviewed May 22, 2023

View reviewed changes

alyssais added the 2.status: merge conflict This PR has merge conflicts with the target branch label May 22, 2023

Artturin reviewed Jun 26, 2023

View reviewed changes

AtnNn force-pushed the emscripten branch from 051f036 to 008cc5c Compare July 6, 2023 00:53

ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 8.has: clean-up and removed 2.status: merge conflict This PR has merge conflicts with the target branch labels Jul 6, 2023

ofborg bot requested a review from qknight July 8, 2023 21:19

ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 10.rebuild-darwin: 11-100 and removed 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ labels Jul 8, 2023

Artturin reviewed Jul 8, 2023

View reviewed changes

sternenseemann reviewed Jul 9, 2023

View reviewed changes

sternenseemann requested a review from a user July 9, 2023 11:30

AtnNn changed the base branch from master to staging July 10, 2023 18:43

github-actions bot added the 6.topic: lib The Nixpkgs function library label Jul 10, 2023

AtnNn force-pushed the emscripten branch from 360185a to 18e939c Compare July 10, 2023 18:43

github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 8.has: changelog labels Jul 10, 2023

AtnNn force-pushed the emscripten branch from 18e939c to 4f7dfda Compare July 10, 2023 19:05

ofborg bot added 10.rebuild-darwin-stdenv This PR causes stdenv to rebuild 10.rebuild-darwin: 501+ 10.rebuild-darwin: 5001+ and removed 10.rebuild-darwin: 11-100 labels Jul 10, 2023

Artturin requested changes Jul 10, 2023

View reviewed changes

emscripten: replace emscriptenPackages with pkgsCross.emscripten

de01f15

AtnNn force-pushed the emscripten branch from 4f7dfda to de01f15 Compare July 11, 2023 00:24

wegank added 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md 2.status: merge conflict This PR has merge conflicts with the target branch labels Mar 19, 2024

stale bot removed the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Mar 20, 2024

alyssais mentioned this pull request Jun 16, 2024

buildRustCrate: add support for compiling to wasm32-unknown-unknown #319153

Merged

13 tasks

wegank added the 2.status: stale https://github.com/NixOS/nixpkgs/blob/master/.github/STALE-BOT.md label Jul 4, 2024

willcohen mentioned this pull request Dec 4, 2024

llvmPackages_19: 19.1.4 -> 19.1.5 #361678

Merged

13 tasks

	emscripten = { execFormat = wasm; families = { }; };
	emscripten = { execFormat = wasm; families = { }; };

	++ (stdenv.extraCmakeFlags or []) ;
	++ (stdenv.extraCmakeFlags or []);

	This mode is far more power full since this makes use of `nix` for
	This mode is far more powerful since this makes use of `nix` for

Add pkgsCross.emscripten, replacing hand-crafted emscriptenPackages #217428

Are you sure you want to change the base?

Add pkgsCross.emscripten, replacing hand-crafted emscriptenPackages #217428

Conversation

AtnNn commented Feb 21, 2023 • edited Loading

Description of changes

Things done

AtnNn commented Feb 21, 2023

RaitoBezarius commented Feb 23, 2023

alyssais left a comment

Choose a reason for hiding this comment

AtnNn commented Mar 25, 2023

mohe2015 commented May 12, 2023

RaitoBezarius commented May 14, 2023

willcohen commented May 19, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willcohen commented May 26, 2023

Choose a reason for hiding this comment

Artturin Jul 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AtnNn commented Jul 10, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

willcohen commented Oct 12, 2023

allsey87 commented Apr 23, 2024 • edited Loading

AtnNn commented Feb 21, 2023 •

edited

Loading

Artturin Jul 8, 2023 •

edited

Loading

allsey87 commented Apr 23, 2024 •

edited

Loading