Link rust_test targets with external linker, fix handlers_test linkage

BUILD.gn

@@ -20,6 +20,16 @@ config("deno_config") {
   if (is_debug) {
     defines = [ "DEBUG" ]
   }
+
+  # Targets built with the `rust_executable()` template automatically pick up
+  # these dependencies, but those built with `executable()` need them when they
+  # have Rust inputs. Currently, there's only one such target, `test_cc`.
+  if (is_mac) {
+    libs = [ "resolv" ]
+  }
+  if (is_win) {
+    libs = [ "userenv.lib" ]
+  }
 }
 
 rust_executable("deno") {
@@ -63,6 +73,21 @@ rust_test("handlers_test") {
     "$rust_build:libc",
     "$rust_build:url",
     "$rust_build:log",
+
+    # Indirect rust depdendencies also need to be listed here:
+    # * Linking to `:handlers` or `:libdeno` isn't possible, because they
+    #   already contain some symbols exported by `handlers.rs`. These duplicate
+    #   symbols trip up the linker.
+    # * The `rust_test` and `rust_executable` templates only produce an object
+    #   file, and then invoke an external linker. Transitive rust depencenies
+    #   are not resolved in either step.
+    "$rust_build:idna",
+    "$rust_build:percent_encoding",
+    "$rust_build:unicode_bidi",
+    "$rust_build:unicode_normalization",
+  ]
+  deps = [
+    ":deno_bindings",
   ]
 }
 
@@ -75,6 +100,7 @@ executable("test_cc") {
   deps = [
     ":deno_base_test",
     ":deno_bindings",
+    ":handlers",
     "//testing/gtest:gtest",
   ]
   configs += [ ":deno_config" ]
@@ -88,6 +114,7 @@ static_library("libdeno") {
   deps = [
     ":create_snapshot_deno",
     ":deno_bindings",
+    ":handlers",
   ]
   configs += [ ":deno_config" ]
 }
@@ -135,7 +162,6 @@ v8_source_set("deno_bindings") {
   ]
   deps = [
     ":deno_base",
-    ":handlers",
     ":msg_cpp",
   ]
   public_deps = [
@@ -202,6 +228,7 @@ source_set("libdeno_nosnapshot") {
   deps = [
     ":bundle",
     ":deno_bindings",
+    ":handlers",
   ]
   configs += [ ":deno_config" ]
   bundle_outputs = get_target_outputs(":bundle")

build_extra/rust/BUILD.gn

@@ -6,14 +6,6 @@ import("rust.gni")
 # Versioning for third party rust crates is controlled in //gclient_config.py
 # TODO(ry) Use Cargo for versioning?
 
-# By compiling an empty file as crate-type=staticlib we get all the code
-# for the rust stdlib, which are not included in the object file outputs
-# of other libs.
-# TODO(ry) This is not used and maybe should be removed along with empty.rs.
-rust_staticlib("stdlib") {
-  source_root = "empty.rs"
-}
-
 crates = "//third_party/rust_crates"
 
 rust_component("libc") {

build_extra/rust/dummy.rs

@@ -0,0 +1 @@
+fn main() {}

build_extra/rust/empty.rs

@@ -1 +0,0 @@
-

build_extra/rust/get_rust_ldflags.cmd

@@ -0,0 +1 @@
+@"%PYTHON_EXE%" "%~dpn0.py" %*

build_extra/rust/get_rust_ldflags.py

@@ -0,0 +1,144 @@
+#!/usr/bin/env python
+# Copyright 2018 Bert Belder <bertbelder@gmail.com>
+# All rights reserved. MIT License.
+
+# The Rust compiler normally builds source code directly into an executable.
+# Internally, object code is produced, and then the (system) linker is called,
+# but this all happens under the covers.
+#
+# However Deno's build system uses it's own linker. For it to successfully
+# produce an executable from rustc-generated object code, it needs to link
+# with a dozen or so "built-in" Rust libraries (as in: not Cargo crates),
+# and we need to tell the linker which and where those .rlibs are.
+#
+# Hard-coding these libraries into the GN configuration isn't possible: the
+# required .rlib files have some sort of hash code in their file name, and their
+# location depends on how Rust is set up, and which toolchain is active.
+#
+# So instead, we have this script: it writes a list of linker options (ldflags)
+# to stdout, separated by newline characters. It is called from `rust.gni` when
+# GN is generating ninja files (it doesn't run in the build phase).
+#
+# There is no official way through which rustc will give us the information
+# we need, so a "back door" is used. We tell `rustc` to compile a (dummy)
+# program, and to use a custom linker. This "linker" doesn't actually link
+# anything; it just dumps it's argv to a temporary file. When rustc is done,
+# this script then reads the linker arguments from that temporary file, and
+# then filters it to remove flags that are irrelevant or undesirable.
+
+import sys
+import os
+from os import path
+import subprocess
+import tempfile
+
+
+def capture_args(argsfile_path):
+    with open(argsfile_path, "wb") as argsfile:
+        argsfile.write("\n".join(sys.argv[1:]))
+
+
+def main():
+    # If ARGSFILE_PATH is set this script is being invoked by rustc, which
+    # thinks we are a linker. All we do now is write our argv to the specified
+    # file and exit. Further processing is done by our grandparent process,
+    # also this script but invoked by gn.
+    argsfile_path = os.getenv("ARGSFILE_PATH")
+    if argsfile_path is not None:
+        return capture_args(argsfile_path)
+
+    # Prepare the environment for rustc.
+    rustc_env = os.environ.copy()
+
+    # We'll capture the arguments rustc passes to the linker by telling it
+    # that this script *is* the linker.
+    # On Posix systems, this file is directly executable thanks to it's shebang.
+    # On Windows, we use a .cmd wrapper file.
+    if os.name == "nt":
+        rustc_linker_base, rustc_linker_ext = path.splitext(__file__)
+        rustc_linker = rustc_linker_base + ".cmd"
+    else:
+        rustc_linker = __file__
+
+    # Make sure that when rustc invokes this script, it uses the same version
+    # of the Python interpreter as we're currently using. On Posix systems this
+    # is done making the Python directory the first element of PATH.
+    # On Windows, the wrapper script uses the PYTHON_EXE environment variable.
+    if os.name == "nt":
+        rustc_env["PYTHON_EXE"] = sys.executable
+    else:
+        python_dir = path.dirname(sys.executable)
+        rustc_env["PATH"] = python_dir + path.pathsep + os.environ["PATH"]
+
+    # Create a temporary file to write captured Rust linker arguments to.
+    # Unfortunately we can't use tempfile.NamedTemporaryFile here, because the
+    # file it creates can't be open in two processes at the same time.
+    argsfile_fd, argsfile_path = tempfile.mkstemp()
+    rustc_env["ARGSFILE_PATH"] = argsfile_path
+
+    try:
+        # Spawn rustc, and make it use this very script as its "linker".
+        rustc_args = ["-Clinker=" + rustc_linker, "-Csave-temps"
+                      ] + sys.argv[1:]
+        subprocess.check_call(["rustc"] + rustc_args, env=rustc_env)
+
+        # Read captured linker arguments from argsfile.
+        argsfile_size = os.fstat(argsfile_fd).st_size
+        argsfile_content = os.read(argsfile_fd, argsfile_size)
+        args = argsfile_content.split("\n")
+
+    finally:
+        # Close and delete the temporary file.
+        os.close(argsfile_fd)
+        os.unlink(argsfile_path)
+
+    # From the list of captured linker arguments, build the list of ldflags that
+    # we actually need.
+    ldflags = []
+    next_arg_is_flag_value = False
+    for arg in args:
+        # Note that within the following if/elif blocks, `pass` means that
+        # that captured arguments gets included in `ldflags`. The final `else`
+        # clause filters out unrecognized/unwanted flags.
+        if next_arg_is_flag_value:
+            # We're looking at a value that follows certain parametric flags,
+            # e.g. the path in '-L <path>'.
+            next_arg_is_flag_value = False
+        elif arg.endswith(".rlib"):
+            # Built-in Rust library, e.g. `libstd-8524caae8408aac2.rlib`.
+            pass
+        elif arg.endswith(".crate.allocator.rcgu.o"):
+            # This file is needed because it contains certain allocator
+            # related symbols (e.g. `__rust_alloc`, `__rust_oom`).
+            # The Rust compiler normally generates this file just before
+            # linking an executable. We pass `-Csave-temps` to rustc so it
+            # doesn't delete the file when it's done linking.
+            pass
+        elif arg.endswith(".lib") and not arg.startswith("msvcrt"):
+            # Include most Windows static/import libraries (e.g. `ws2_32.lib`).
+            # However we ignore Rusts choice of C runtime (`mvcrt*.lib`).
+            # Rust insists on always using the release "flavor", even in debug
+            # mode, which causes conflicts with other libraries we link with.
+            pass
+        elif arg.upper().startswith("/LIBPATH:"):
+            # `/LIBPATH:<path>`: Linker search path (Microsoft style).
+            pass
+        elif arg == "-l" or arg == "-L":
+            # `-l <name>`: Link with library (GCC style).
+            # `-L <path>`: Linker search path (GCC style).
+            next_arg_is_flag_value = True  # Ensure flag argument is captured.
+        elif arg == "-Wl,--start-group" or arg == "-Wl,--end-group":
+            # Start or end of an archive group (GCC style).
+            pass
+        else:
+            # Not a flag we're interested in -- don't add it to ldflags.
+            continue
+
+        ldflags += [arg]
+
+    # Write the filtered ldflags to stdout, separated by newline characters.
+    sys.stdout.write("\n".join(ldflags))
+
+
+if __name__ == '__main__':
+    sys.exit(main())

build_extra/rust/rust.gni

@@ -1,5 +1,3 @@
-stdlib_label = "//build_extra/rust:stdlib"
-
 declare_args() {
   # Absolute path of rust build files.
   rust_build = "//build_extra/rust/"
@@ -11,26 +9,52 @@ if (is_win) {
   executable_suffix = ""
 }
 
+# The official way of building Rust executables is to to let rustc do the
+# linking. However, we'd prefer to leave it in the hands of gn/ninja:
+#   * It allows us to use source sets.
+#   * It allows us to use the bundled lld that Chromium and V8 use.
+#   * We have more control over build flags.
+#   * To sidestep rustc weirdness (e.g. on Windows, it always links with the
+#     release C runtime library, even for debug builds).
+#
+# The `get_rust_ldflags` tool outputs the linker flags that are needed to
+# successfully link rustc object code into an executable.
+# We generate two sets of ldflags:
+#   `rust_bin_ldflags`:  Used for rust_executable targets.
+#   `rust_test_ldflags`: Used for rust_test targets; includes the test harness.
+#
+# The tool works by compiling and linking something with rustc, and analyzing
+# the arguments it passes to the system linker. That's what dummy.rs is for.
+dummy_rs_path = rebase_path("dummy.rs", root_build_dir)
+rust_bin_ldflags =
+    exec_script("get_rust_ldflags.py", [ dummy_rs_path ], "list lines")
+rust_test_ldflags = exec_script("get_rust_ldflags.py",
+                                [
+                                  dummy_rs_path,
+                                  "--test",
+                                ],
+                                "list lines")
+
 template("run_rustc") {
   action(target_name) {
     assert(defined(invoker.source_root), "Must specify source_root")
     forward_variables_from(invoker,
                            [
                              "cfg",
+                             "crate_name",
                              "crate_type",
                              "source_root",
                              "deps",
                              "extern",
                              "is_test",
+                             "testonly",
                            ])
-    if (defined(invoker.testonly)) {
-      testonly = invoker.testonly
-    }
-    if (defined(invoker.crate_name)) {
-      crate_name = invoker.crate_name
-    } else {
+    if (!defined(crate_name)) {
       crate_name = target_name
     }
+    if (!defined(is_test)) {
+      is_test = false
+    }
 
     sources = [
       source_root,
@@ -47,18 +71,6 @@ template("run_rustc") {
       args += [ "--color=always" ]
     }
 
-    if (defined(is_test) && is_test) {
-      # Test outputs are executables which should be in root_out_dir.
-      output_file = "$root_out_dir/$crate_name" + executable_suffix
-      args += [
-        "--test",
-        "-o",
-        rebase_path(output_file, root_build_dir),
-      ]
-      outputs += [ output_file ]
-    } else {
-      # Non-test targets are handled differently.
-
     if (crate_type == "staticlib") {
       output_file = "$target_out_dir/$crate_name.a"
       emit_type = "link"
@@ -73,7 +85,6 @@ template("run_rustc") {
     output_file_rel = rebase_path(output_file, root_build_dir)
     args += [ "--emit=$emit_type=$output_file_rel" ]
 
-      # TODO(ry) For unknown reasons emitting a depfile on tests doesn't work.
     depfile = "$target_out_dir/$crate_name.d"
     args += [
       "--emit=dep-info=" + rebase_path(depfile, root_build_dir),
@@ -83,7 +94,6 @@ template("run_rustc") {
       "--depfile=" + rebase_path(depfile, root_build_dir),
       "--output_file=" + output_file_rel,
     ]
-    }
 
     if (is_debug) {
       args += [ "-g" ]
@@ -93,6 +103,10 @@ template("run_rustc") {
       args += [ "-O" ]
     }
 
+    if (is_test) {
+      args += [ "--test" ]
+    }
+
     if (defined(cfg)) {
       foreach(c, cfg) {
         args += [
@@ -139,6 +153,7 @@ template("rust_component") {
                              "extern",
                              "cfg",
                              "source_root",
+                             "is_test",
                              "testonly",
                            ])
     if (!defined(invoker.crate_type)) {
@@ -169,7 +184,6 @@ template("rust_component") {
 
 template("rust_staticlib") {
   rust_component(target_name) {
-    crate_type = "staticlib"
     forward_variables_from(invoker,
                            [
                              "crate_name",
@@ -178,12 +192,7 @@ template("rust_staticlib") {
                              "source_root",
                              "testonly",
                            ])
-    if (current_os == "mac") {
-      libs = [ "resolv" ]
-    }
-    if (current_os == "win") {
-      libs = [ "userenv.lib" ]
-    }
+    crate_type = "staticlib"
   }
 }
 
@@ -199,14 +208,17 @@ template("rust_executable") {
   executable(target_name) {
     forward_variables_from(invoker, "*")
 
+    if (defined(is_test) && is_test) {
+      ldflags = rust_test_ldflags
+    } else {
+      ldflags = rust_bin_ldflags
+    }
+
     if (!defined(deps)) {
       deps = []
     }
 
-    deps += [
-      bin_label,
-      stdlib_label,
-    ]
+    deps += [ bin_label ]
 
     if (defined(extern)) {
       deps += extern
@@ -215,16 +227,9 @@ template("rust_executable") {
 }
 
 template("rust_test") {
-  run_rustc(target_name) {
-    crate_name = target_name
-    crate_type = "bin"
-    testonly = true
+  rust_executable(target_name) {
+    forward_variables_from(invoker, "*")
     is_test = true
-    forward_variables_from(invoker,
-                           [
-                             "extern",
-                             "cfg",
-                             "source_root",
-                           ])
+    testonly = true
   }
 }

tools/format.py

@@ -16,7 +16,7 @@ run(["clang-format", "-i", "-style", "Google"] + glob("src/*.cc") +
 for fn in ["BUILD.gn", ".gn"] + glob("build_extra/**/*.gn*"):
     run(["gn", "format", fn])
 # TODO(ry) Install yapf in third_party.
-run(["yapf", "-i"] + glob("tools/*.py"))
+run(["yapf", "-i"] + glob("tools/*.py") + glob("build_extra/**/*.py"))
 run(["node", prettier, "--write"] + glob("js/*.js") + glob("js/*.ts") +
     ["tsconfig.json"] + ["tslint.json"])