-
Notifications
You must be signed in to change notification settings - Fork 3.4k
Extend embuilder deferred building mode to ports #23924
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
Changes from 5 commits
021e0cc
ba2b0d6
f6e3bdd
eb3101c
830eddb
7082238
060947e
0ad6f2c
36090e3
94648d8
6d36a65
75b1151
3e52a80
ca3585e
a4d161e
031ff61
6faa8a5
1772acd
a6944b4
cfcde08
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -6,6 +6,7 @@ | |
import logging | ||
import hashlib | ||
import os | ||
from pathlib import Path | ||
import shutil | ||
import glob | ||
import importlib.util | ||
|
@@ -36,6 +37,8 @@ | |
|
||
logger = logging.getLogger('ports') | ||
|
||
build_deferred = False | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 |
||
|
||
|
||
def init_port(name, port): | ||
ports.append(port) | ||
|
@@ -176,7 +179,9 @@ def install_headers(src_dir, pattern='*.h', target=None): | |
|
||
@staticmethod | ||
def build_port(src_dir, output_path, port_name, includes=[], flags=[], cxxflags=[], exclude_files=[], exclude_dirs=[], srcs=[]): # noqa | ||
build_dir = os.path.join(Ports.get_build_dir(), port_name) | ||
mangled_name = str(Path(output_path).relative_to(cache.get_sysroot(True))).replace(os.sep, '_') | ||
build_dir = os.path.join(Ports.get_build_dir(), mangled_name) | ||
logger.debug(f'build_port: {port_name} {output_path} in {build_dir}') | ||
if srcs: | ||
srcs = [os.path.join(src_dir, s) for s in srcs] | ||
else: | ||
|
@@ -199,7 +204,8 @@ def build_port(src_dir, output_path, port_name, includes=[], flags=[], cxxflags= | |
ninja_file = os.path.join(build_dir, 'build.ninja') | ||
system_libs.ensure_sysroot() | ||
system_libs.create_ninja_file(srcs, ninja_file, output_path, cflags=cflags) | ||
system_libs.run_ninja(build_dir) | ||
if not build_deferred: | ||
system_libs.run_ninja(build_dir) | ||
else: | ||
commands = [] | ||
objects = [] | ||
|
@@ -525,7 +531,10 @@ def get_needed_ports(settings): | |
return needed | ||
|
||
|
||
def build_port(port_name, settings): | ||
def build_port(port_name, settings, deferred=False): | ||
global build_deferred | ||
build_deferred = deferred | ||
logger.debug(f'build_port {port_name}, {"deferred" if deferred else ""}') | ||
port = ports_by_name[port_name] | ||
port_set = OrderedSet([port]) | ||
resolve_dependencies(port_set, settings) | ||
|
@@ -573,10 +582,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 | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. So this use of So emcc always installs headers in 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? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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 There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this works; WDYT? |
||
for port in dependency_order(needed): | ||
port.get(Ports, settings, shared) | ||
args += port.process_args(Ports) | ||
|
||
build_deferred = False | ||
|
||
def show_ports(): | ||
sorted_ports = sorted(ports, key=lambda p: p.name) | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -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' | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 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. |
||
ports.fetch_project('sdl2_gfx', f'https://github.com/svn2github/sdl2_gfx/archive/{TAG}.zip', sha512hash=HASH) | ||
|
||
def create(final): | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we make this directory
cache/build/ports
instead?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.