Improve SCons support (#18945)

Made it so that `emscons` also provides the `EMSCRIPTEN_ROOT`
environment variable and made the SCons tool provide the environment
variables that `emmake` (and others) does.

I was in the spirit of #18279 and started making the SCons tool search
for the root directory by scanning the `PATH` instead of relying on that
variable, but then I realised that it would be much simpler for
`emscons` to provide it.

I also noted that, unlike `emmake` and `emconfigure`, the `emscons`
command would not set some environment variables — notably to me, the
`pkg-config`-related ones.

Because the default SCons behaviour is to not propagate environment
variables to child processes, it made little sense to add them in
`emscons.py`. Instead I made the SCons tool directly import the relevant
Python package from the root directory and get only the relevant
environment variables from there, so that it would not touch any other.

For that I had to create a different function, because
`get_building_env()` returns the inherited environment variables as
well.

For the tests I made the SCons script aware of what environment
variables to expect by passing to it an argument containing them in JSON
form. Is that too hacky?

Author: veigaribo

ChangeLog.md

@@ -20,6 +20,12 @@ See docs/process.md for more on how version tagging works.
 
 3.1.64 (in development)
 -----------------------
+- Updated the SCons tool to not require the `EMSCRIPTEN_ROOT` environment
+  variable, in which case it will assume that SCons will find the binaries in
+  (its) `PATH`.
+- Updated `emscons` to apply the `EMSCRIPTEN_ROOT`, `EMSCONS_PKG_CONFIG_LIBDIR`
+  and `EMSCONS_PKG_CONFIG_PATH` environment variables. The SCons tool will use
+  last two to set up `PKG_CONFIG_LIBDIR` and `PKG_CONFIG_PATH` respectively.
 
 3.1.63 - 07/12/24
 -----------------

emscons.py

@@ -11,11 +11,15 @@
 import os
 import subprocess
 import sys
-from tools import utils
+from tools import building, utils
 
 tool_path = utils.path_from_root('tools/scons/site_scons/site_tools/emscripten')
+building_env = building.get_building_env()
 
 env = os.environ.copy()
 env['EMSCRIPTEN_TOOL_PATH'] = tool_path
+env['EMSCRIPTEN_ROOT'] = utils.path_from_root()
+env['EMSCONS_PKG_CONFIG_LIBDIR'] = building_env['PKG_CONFIG_LIBDIR']
+env['EMSCONS_PKG_CONFIG_PATH'] = building_env['PKG_CONFIG_PATH']
 
 sys.exit(subprocess.call(sys.argv[1:], env=env))

test/scons/env/SConstruct

@@ -0,0 +1,34 @@
+import json
+import os
+
+
+def assert_(value, expected):
+  if value != expected:
+    print("assert failed:", value, expected)
+    Exit(1)
+
+
+env = Environment(toolpath=[os.environ.get('EMSCRIPTEN_TOOL_PATH')])
+env['ENV'] = dict()
+
+env.Tool('emscripten')
+
+AddOption(
+  '--expected-env',
+  dest='expected',
+  type='string',
+  nargs=1,
+  action='store',
+  help='JSON with the expected environment parameters',
+)
+
+expected_env = json.loads(GetOption('expected'))
+expected_env_env = expected_env.pop('ENV')
+
+for key in expected_env_env:
+  assert_(env['ENV'].get(key), expected_env_env[key])
+
+for key in expected_env:
+  assert_(env.get(key), expected_env[key])
+
+Exit(0)

test/scons/SConstruct renamed to test/scons/simple/SConstruct

@@ -27,6 +27,7 @@
 if __name__ == '__main__':
   raise Exception('do not run this file directly; do something like: test/runner other')
 
+from tools.building import get_building_env
 from tools.shared import config
 from tools.shared import EMCC, EMXX, EMAR, EMRANLIB, FILE_PACKAGER, LLVM_NM
 from tools.shared import CLANG_CC, CLANG_CXX, LLVM_AR, LLVM_DWARFDUMP, LLVM_DWP, EMCMAKE, EMCONFIGURE, WASM_LD
@@ -3038,25 +3039,84 @@ def test_dwarf_sourcemap_names(self):
   @with_env_modify({'EMSCRIPTEN_ROOT': path_from_root()})
   def test_scons(self):
     # this test copies the site_scons directory alongside the test
-    shutil.copytree(test_file('scons'), 'test')
+    shutil.copytree(test_file('scons/simple'), 'test')
     shutil.copytree(path_from_root('tools/scons/site_scons'), Path('test/site_scons'))
     with utils.chdir('test'):
       self.run_process(['scons'])
       output = self.run_js('scons_integration.js', assert_returncode=5)
     self.assertContained('If you see this - the world is all right!', output)
 
   @requires_scons
-  @with_env_modify({'EMSCRIPTEN_TOOLPATH': path_from_root('tools/scons/site_scons'),
-                    'EMSCRIPTEN_ROOT': path_from_root()})
+  @with_env_modify({
+    'EMSCRIPTEN_ROOT': path_from_root(),
+    'EMSCONS_PKG_CONFIG_LIBDIR': '/pkg/config/libdir',
+    'EMSCONS_PKG_CONFIG_PATH': '/pkg/config/path',
+  })
+  def test_scons_env(self):
+    # this test copies the site_scons directory alongside the test
+    shutil.copytree(test_file('scons/env'), 'test')
+    shutil.copytree(path_from_root('tools/scons/site_scons'), Path('test/site_scons'))
+
+    expected_to_propagate = json.dumps({
+      'CC': path_from_root('emcc'),
+      'CXX': path_from_root('em++'),
+      'AR': path_from_root('emar'),
+      'RANLIB': path_from_root('emranlib'),
+      'ENV': {
+        'PKG_CONFIG_LIBDIR': '/pkg/config/libdir',
+        'PKG_CONFIG_PATH': '/pkg/config/path',
+      }
+    })
+
+    with utils.chdir('test'):
+      self.run_process(['scons', '--expected-env', expected_to_propagate])
+
+  @requires_scons
+  def test_scons_env_no_emscons(self):
+    shutil.copytree(test_file('scons/env'), 'test')
+    shutil.copytree(path_from_root('tools/scons/site_scons'), Path('test/site_scons'))
+
+    expected_to_propagate = json.dumps({
+      'CC': 'emcc',
+      'CXX': 'em++',
+      'AR': 'emar',
+      'RANLIB': 'emranlib',
+      'ENV': {
+        'PKG_CONFIG_LIBDIR': None,
+        'PKG_CONFIG_PATH': None,
+      }
+    })
+
+    with utils.chdir('test'):
+      self.run_process(['scons', '--expected-env', expected_to_propagate])
+
+  @requires_scons
   def test_emscons(self):
-    # uses the emscons wrapper which requires EMSCRIPTEN_TOOLPATH to find
-    # site_scons
-    shutil.copytree(test_file('scons'), 'test')
+    shutil.copytree(test_file('scons/simple'), 'test')
     with utils.chdir('test'):
       self.run_process([path_from_root('emscons'), 'scons'])
       output = self.run_js('scons_integration.js', assert_returncode=5)
     self.assertContained('If you see this - the world is all right!', output)
 
+  @requires_scons
+  def test_emscons_env(self):
+    shutil.copytree(test_file('scons/env'), 'test')
+
+    building_env = get_building_env()
+    expected_to_propagate = json.dumps({
+      'CC': path_from_root('emcc'),
+      'CXX': path_from_root('em++'),
+      'AR': path_from_root('emar'),
+      'RANLIB': path_from_root('emranlib'),
+      'ENV': {
+        'PKG_CONFIG_LIBDIR': building_env['PKG_CONFIG_LIBDIR'],
+        'PKG_CONFIG_PATH': building_env['PKG_CONFIG_PATH'],
+      }
+    })
+
+    with utils.chdir('test'):
+      self.run_process([path_from_root('emscons'), 'scons', '--expected-env', expected_to_propagate])
+
   def test_embind_fail(self):
     out = self.expect_fail([EMXX, test_file('embind/test_unsigned.cpp')])
     self.assertContained("undefined symbol: _embind_register_function", out)

test/scons/integration.cpp renamed to test/scons/simple/integration.cpp

@@ -8,48 +8,60 @@
 import os
 
 
-def generate(env, emscripten_path=None, **kw):
-  """ SCons tool entry point """
+def path_to_bin(emscripten_root, bin_name):
+  if emscripten_root is None:
+    return bin_name
+  else:
+    return os.path.join(emscripten_root, bin_name)
+
 
-  if emscripten_path is None:
-    emscripten_path = os.environ.get('EMSCRIPTEN_ROOT')
-    if not emscripten_path:
-      raise 'Unable to find emscripten. Please set EMSCRIPTEN_ROOT'
+def generate(env, **kw):
+  """ SCons tool entry point """
 
   # SCons does not by default invoke the compiler with the
-  # environment variabls from the parent calling process,
+  # environment variables from the parent calling process,
   # so manually route all environment variables referenced
   # by Emscripten to the child.
-  for var in ['EM_CACHE', 'EMCC_DEBUG', 'EM_CONFIG',
-              'EMMAKEN_JUST_CONFIGURE', 'EMCC_CFLAGS', 'EMCC_TEMP_DIR',
-              'EMCC_AUTODEBUG', 'EM_COMPILER_WRAPPER',
-              'MOZ_DISABLE_AUTO_SAFE_MODE', 'EMCC_STDERR_FILE',
-              'EMSCRIPTEN_SUPPRESS_USAGE_WARNING', 'NODE_PATH', 'EMCC_JSOPT_MIN_CHUNK_SIZE',
-              'EMCC_JSOPT_MAX_CHUNK_SIZE', 'EMCC_CORES', 'EMCC_NO_OPT_SORT',
-              'EMCC_BUILD_DIR', 'EMCC_DEBUG_SAVE', 'EMCC_SKIP_SANITY_CHECK',
-              'EM_PKG_CONFIG_PATH', 'EMCC_CLOSURE_ARGS',
-              'EMCC_FORCE_STDLIBS', 'EMCC_ONLY_FORCED_STDLIBS', 'EM_PORTS', 'IDL_CHECKS', 'IDL_VERBOSE']:
-    if os.environ.get(var):
-      env['ENV'][var] = os.environ.get(var)
-  try:
-    emscPath = emscripten_path.abspath
-  except:
-    emscPath = emscripten_path
-
-  env.Replace(CC=os.path.join(emscPath, "emcc"))
-  env.Replace(CXX=os.path.join(emscPath, "em++"))
+  emscripten_env_vars = ['EM_CACHE', 'EMCC_DEBUG', 'EM_CONFIG',
+    'EMMAKEN_JUST_CONFIGURE', 'EMCC_CFLAGS', 'EMCC_TEMP_DIR',
+    'EMCC_AUTODEBUG', 'EM_COMPILER_WRAPPER', 'MOZ_DISABLE_AUTO_SAFE_MODE',
+    'EMCC_STDERR_FILE', 'EMSCRIPTEN_SUPPRESS_USAGE_WARNING', 'NODE_PATH',
+    'EMCC_JSOPT_MIN_CHUNK_SIZE', 'EMCC_JSOPT_MAX_CHUNK_SIZE',
+    'EMCC_CORES', 'EMCC_NO_OPT_SORT', 'EMCC_BUILD_DIR',
+    'EMCC_DEBUG_SAVE', 'EMCC_SKIP_SANITY_CHECK', 'EM_PKG_CONFIG_PATH',
+    'EMCC_CLOSURE_ARGS', 'EMCC_FORCE_STDLIBS',
+    'EMCC_ONLY_FORCED_STDLIBS', 'EM_PORTS', 'IDL_CHECKS', 'IDL_VERBOSE']
+
+  pkg_config_vars = {
+    'EMSCONS_PKG_CONFIG_LIBDIR': 'PKG_CONFIG_LIBDIR',
+    'EMSCONS_PKG_CONFIG_PATH': 'PKG_CONFIG_PATH',
+  }
+
+  for var in emscripten_env_vars:
+    if var in os.environ:
+      env['ENV'][var] = os.environ[var]
+
+  for var in pkg_config_vars:
+    if var in os.environ:
+      real_key = pkg_config_vars[var]
+      env['ENV'][real_key] = os.environ[var]
+
+  # Binary paths will be constructed from here if available.
+  # Otherwise they are assumed to be in the PATH.
+  emscripten_root = os.environ.get('EMSCRIPTEN_ROOT')
+
+  env.Replace(CC=path_to_bin(emscripten_root, 'emcc'))
+  env.Replace(CXX=path_to_bin(emscripten_root, 'em++'))
   # LINK uses smark_link by default which will choose
   # either emcc or em++ depending on if there are any C++ sources
   # in the program, so no need to change that.
   # SHLINK and LDMODULE should use LINK so no
   # need to change them here
 
-  env.Replace(AR=os.path.join(emscPath, "emar"))
-  env.Replace(RANLIB=os.path.join(emscPath, "emranlib"))
+  env.Replace(AR=path_to_bin(emscripten_root, 'emar'))
+  env.Replace(RANLIB=path_to_bin(emscripten_root, 'emranlib'))
 
-  env.Replace(OBJSUFFIX=[".js", ".bc", ".o"][2])
-  env.Replace(LIBSUFFIX=[".js", ".bc", ".o"][2])
-  env.Replace(PROGSUFFIX=[".html", ".js"][1])
+  env.Replace(PROGSUFFIX='.js')
 
 
 def exists(env):