Extend embuilder deferred building mode to ports #23924

dschuff · 2025-03-14T00:00:09Z

This allows embuilder build ALL to mix the ports builds together with the
system_libs builds, for even faster build-linux speed.
Headers are still installed eagerly during ports.get() (to allow dependencies to
build their object files) but does not run ninja to build the ports until
the explicit step at the end.

This version requires threading the 'deferred' flag through each port definition.

dschuff · 2025-03-14T00:02:03Z

tools/ports/__init__.py

@@ -36,6 +36,7 @@

 logger = logging.getLogger('ports')

+build_deferred = False


This global variable is ugly but it allows this approach to work without having to edit all the ports definition files. The alternative would be to thread the deferred argument through the get() of each port and its calls of ports.build_port.

tools/cache.py

tools/ports/__init__.py

…ries

Split out from emscripten-core#23924

dschuff · 2025-03-14T23:11:54Z

tools/ports/sdl2_gfx.py

@@ -16,8 +16,6 @@ def needed(settings):


 def get(ports, settings, shared):
-  sdl_build = os.path.join(ports.get_build_dir(), 'sdl2')
-  assert os.path.exists(sdl_build), 'You must use SDL2 to use SDL2_gfx'


This doesn't work when the actual library build is deferred, but the dependency code in ports/init.py should ensure that the dependency is met.

dschuff · 2025-03-15T00:15:35Z

This does seem to work now, and cut about 4 minutes off of the build-linux stage on the critical path. seems like it might be worthwhile to consider?

Split out from emscripten-core#23924

Split out from #23924

sbc100 · 2025-03-14T18:36:39Z

.circleci/config.yml

@@ -499,7 +499,7 @@ jobs:
      - build-libs
      - run:
          name: Clean build directory
-          command: rm -rf ~/cache/build
+          command: rm -rf ~/cache/build ~/cache/ports-builds


Should we make this directory cache/build/ports instead?

I could go either way on that. After #23932 they are all just together under /build in their own uniquely-named directory, which seems fine to me.

embuilder.py

sbc100 · 2025-03-14T18:44:25Z

tools/cache.py

@@ -129,6 +129,10 @@ def get_lib_dir(absolute):
  return path


+def get_lib_subdir():
+  return get_lib_dir(False).relative_to(Path(get_sysroot(False), 'lib'))


Could we do this the other way around? i.e put the subdir calculation logic here in this function and then have get_lib_dir call get_lib_subdir?

def get_lib_dir(absolute): ensure_setup() libroot = Path(get_sysroot(absolute=absolute), 'lib') return Path(libroot, get_lib_subdir()

This is removed in the current version.

embuilder.py

This allows building them simultaneously with Ninja. Split out from emscripten-core#23924

This allows building them simultaneously with Ninja. Split out from #23924

sbc100 · 2025-03-21T16:57:16Z

tools/ports/__init__.py

@@ -578,9 +584,13 @@ def add_cflags(args, settings): # noqa: U100

  # Now get (i.e. build) the ports in dependency order.  This is important because the
  # headers from one ports might be needed before we can build the next.
+  global build_deferred
+  assert not build_deferred, "add_cflags shouldn't be called from embuilder"
+  build_deferred = True


When are the deferred ports actually built?

If we defer the building of the port does when only compiling does that mean that the port headers will be installed N times if we do emcc -c -sUSE_SDL2 for each source file, and then once again at link time?

Deferred libs and ports are only built when using embuilder.py. It explicitly invokes ninja (via system_libs.build_deferred()) after all the calls to port.build.
emcc never builds anything deferred.

So this code does nothing outside of embuilder?

Is it still needed? I find the assertion a little strange. If add_cflags is never called from embuilder then why are we needing to set build_deferred here (which means nothing outside of embuilder).

Ah yes, so here's the issue. The headers for any dependencies need to be in place in order to build object files. So (as the comment on top of the function suggests), this can normally cause those dependencies to be built to get the headers in place. When building with embuilder we want all the object files to be deferred and built by the single invocation of ninja at the end (i.e. we don't want the invocations of emcc that build the object files to cause additional object files and archives to be built). But emcc is in a different process from embuilder, so it doesn't have access to the build_deferred global in the embuilder process. So what was happening was that some of the libraries were getting built twice (once as a result of add_cflags inside an invocation of emcc, and once from the top-level ninja file).

So this use of build_deferred is for emcc usage and not for embuilder usage?

So emcc always installs headers in build_deffered mode now?

Doesn't that mean that if I compile 1000 source files I will get the headers for any ports use installed 1000 separate times? Since none of the compile steps actually build the library (due to being deffered) each compile will run as if the library is completely missing and go ahead and install the headers, no?

Locally it doesn't seem like cache lock contention is too much of an issue (after the port is unpacked, which only happens ones) but I guess the create function is called under the lock, so the header check happens with the lock. Another env variable would certainly do the trick. It could be scoped to skipping ports as you suggested, or it could be used more generally to mean that we are running an embuilder library build. It coudl just replace the build_deferred global variable.

Its too gross for me I think.

At the very least we should somehow ensure this behaviour change is only for embuilder. As it stands this would effect all USE_NINJA=1 users. How about a separate USE_DEFFERED_BUILD=1 or something like that that embuilder can set?

Yeah, I was initially not thrilled about another env var, but the fact that it can replace all the uses of this global is nice. I'll go with that.

I thought about this a little more. Using the env var in place of the global var here does make it a bit nicer. But I realized that we don't actually need that in order to keep the headers from being installed redundantly. we just need to omit the ports.get for the dependency ports under embuilder. It's nicer.

I think this works; WDYT?

sbc100 · 2025-03-31T20:51:05Z

embuilder.py

@@ -280,6 +281,9 @@ def main():
  if auto_tasks:
    print('Building targets: %s' % ' '.join(tasks))

+  if USE_NINJA:
+    os.environ['EMBUILDER_PORT_BUILD_DEFERRED'] = '1'


So its not currently possible use ninja in non-deferred mode? I guess thats OK?

Ninja still works in non-deferred mode. If you do EMCC_USE_NINJA=1 test/runner.py core0 it will build the libraries on-demand in non-deferred mode with ninja. Deferred mode only makes sense when using embuilder though.

tools/ports/__init__.py

dschuff · 2025-04-04T00:31:48Z

Any more thoughts or questions on this one?

dschuff · 2025-04-23T22:15:13Z

The remaining failures here are the flaky SDL mouse and audio tests we've been seeing. Let's either land this or decide to kill it.

sbc100

lgtm % nits

sbc100 · 2025-04-23T22:24:15Z

embuilder.py

-  with get_port_variant(port_name) as port_name:
-    ports.build_port(port_name, settings)
+  with get_port_variant(port_name) as port_name_base:
+    ports.build_port(port_name_base, settings)


I think this is maybe more like "full_name" ?

sbc100 · 2025-04-23T22:26:24Z

tools/cache.py

-    if not deferred:
-      assert cachename.exists()
+    if not os.getenv('EMBUILDER_PORT_BUILD_DEFERRED'):
+      assert cachename.is_file()


I would rather have this check done in tools/ports/__init__.py if possible and continue to pass
deferred here as an argument.

The rational being that EMBUILDER_PORT_BUILD_DEFERRED should logically only effect ports and I don't really want post-specific stuff here in the general purpose caching code.

Yeah, that does make sense; the only problem is that cache.get() is called (via cache.get_lib()) from every port build script. So we'd have to update all of them to pass through the deferred argument manually. Or else come up with some other weird solution (like making a fake/empty archive file that will pass the assertion but then get overwritten when we run ninja or something).

Fair enough LGTM. Maybe add a comment as to why this needs to be here.

I can do that (the only other idea I had that didn't involve updating all the port files was creating an empty archive file that would get overwritten when ninja was run. That seemed worse to me than just having the file not be there, but if you'd prefer that let me know).

dschuff added 2 commits March 13, 2025 18:34

Allow deferred/combined building of ports with system_libs.

021e0cc

This version requires threading the 'deferred' flag through each port definition.

use a global in cache/__init__.py instead

ba2b0d6

dschuff commented Mar 14, 2025

View reviewed changes

tools/cache.py Show resolved Hide resolved

Use separate build directories for ports variants

f6e3bdd

dschuff commented Mar 14, 2025

View reviewed changes

tools/ports/__init__.py Outdated Show resolved Hide resolved

dschuff added 2 commits March 14, 2025 18:39

clean up, work around lack of str.removeprefix

eb3101c

Use lib archive path/name instead of variant name to separate directo…

830eddb

…ries

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 14, 2025

Use common build directory for ports and system libs. NFC

eb5c5bc

Split out from emscripten-core#23924

sbc100 mentioned this pull request Mar 14, 2025

Use common build directory for ports and system libs. NFC #23932

Merged

dschuff commented Mar 14, 2025

View reviewed changes

remove unused argument

7082238

dschuff marked this pull request as ready for review March 15, 2025 00:15

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 17, 2025

Use common build directory for ports and system libs. NFC

e0b8410

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 17, 2025

Use common build directory for ports and system libs. NFC

f95ec90

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC

49bce1a

Split out from emscripten-core#23924

sbc100 added a commit to sbc100/emscripten that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC

33114f1

Split out from emscripten-core#23924

sbc100 added a commit that referenced this pull request Mar 19, 2025

Use common build directory for ports and system libs. NFC (#23932)

5317e44

Split out from #23924

dschuff added 3 commits March 20, 2025 18:04

Merge branch 'main' into ports-deferred

060947e

remove unnecessary file deletion

0ad6f2c

fix ruff

36090e3

sbc100 reviewed Mar 20, 2025

View reviewed changes

remove unneeded ensure_dirs

94648d8

dschuff added a commit to dschuff/emscripten that referenced this pull request Mar 20, 2025

Use a separate build directory for each port

03c1930

This allows building them simultaneously with Ninja. Split out from emscripten-core#23924

dschuff mentioned this pull request Mar 20, 2025

Use a separate build directory for each port #23961

Merged

dschuff added a commit that referenced this pull request Mar 21, 2025

Use a separate build directory for each port (#23961)

a5af114

This allows building them simultaneously with Ninja. Split out from #23924

Merge branch 'main' into ports-deferred

6d36a65

sbc100 reviewed Mar 21, 2025

View reviewed changes

Use global

75b1151

dschuff added 4 commits March 25, 2025 23:44

skip ports.get to avoid redundant header install

3e52a80

skip ports.get when embuilder

ca3585e

Merge branch 'main' into ports-deferred

a4d161e

Merge branch 'main' into ports-deferred

031ff61

sbc100 reviewed Mar 31, 2025

View reviewed changes

tools/ports/__init__.py Show resolved Hide resolved

Merge branch 'main' into ports-deferred

6faa8a5

dschuff force-pushed the ports-deferred branch from e68e0db to 6faa8a5 Compare April 3, 2025 01:02

dschuff added 2 commits April 7, 2025 19:58

Put back assertion

1772acd

Merge branch 'main' into ports-deferred

a6944b4

sbc100 approved these changes Apr 23, 2025

View reviewed changes

Add comment

cfcde08

sbc100 approved these changes Apr 24, 2025

View reviewed changes

dschuff enabled auto-merge (squash) April 24, 2025 16:48

dschuff disabled auto-merge April 24, 2025 19:44

dschuff merged commit c57d2a9 into emscripten-core:main Apr 24, 2025
26 of 28 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Extend embuilder deferred building mode to ports #23924

Extend embuilder deferred building mode to ports #23924

dschuff commented Mar 14, 2025 •

edited

Loading

dschuff Mar 14, 2025

dschuff Mar 14, 2025

dschuff commented Mar 15, 2025

sbc100 Mar 14, 2025

dschuff Mar 20, 2025

sbc100 Mar 14, 2025

dschuff Mar 20, 2025

sbc100 Mar 21, 2025

dschuff Mar 21, 2025

sbc100 Mar 21, 2025

dschuff Mar 21, 2025

sbc100 Mar 21, 2025

dschuff Mar 22, 2025

sbc100 Mar 22, 2025

dschuff Mar 22, 2025 •

edited

Loading

dschuff Mar 25, 2025

dschuff Mar 26, 2025

sbc100 Mar 31, 2025

dschuff Apr 2, 2025

dschuff commented Apr 4, 2025

dschuff commented Apr 23, 2025

sbc100 left a comment

sbc100 Apr 23, 2025

sbc100 Apr 23, 2025

dschuff Apr 23, 2025

sbc100 Apr 23, 2025

dschuff Apr 24, 2025

		@@ -36,6 +36,7 @@

		logger = logging.getLogger('ports')

		build_deferred = False

Extend embuilder deferred building mode to ports #23924

Extend embuilder deferred building mode to ports #23924

Conversation

dschuff commented Mar 14, 2025 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Mar 15, 2025

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

dschuff Mar 22, 2025 • 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

dschuff commented Apr 4, 2025

dschuff commented Apr 23, 2025

sbc100 left a 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

dschuff commented Mar 14, 2025 •

edited

Loading

dschuff Mar 22, 2025 •

edited

Loading