fix: devserver not working on windows refactor: remove unnecessary runfile stat Closes #385 PiperOrigin-RevId: 233100030
diff --git a/.bazelci/presubmit.yml b/.bazelci/presubmit.yml index f587cc9..5b60a2c 100644 --- a/.bazelci/presubmit.yml +++ b/.bazelci/presubmit.yml
@@ -52,6 +52,4 @@ - "--action_env=PATH" - "--test_env=PATH" test_targets: - - "--" - "..." - - "-//devserver/devserver:go_default_test"
diff --git a/devserver/devserver/devserver.go b/devserver/devserver/devserver.go index 4748908..9bd3a86 100644 --- a/devserver/devserver/devserver.go +++ b/devserver/devserver/devserver.go
@@ -97,13 +97,7 @@ func CreateFileHandler(servingPath, manifest string, pkgs []string, base string) http.HandlerFunc { // We want to add the root runfile path because by default developers should be able to request // runfiles through their absolute manifest path (e.g. "my_workspace_name/src/file.css") - // We use the empty string package because of the different algorithm for - // file resolution used internally and externally. - pkgPaths := dirHTTPFileSystem{[]string{"./"}, base} - for _, pkg := range pkgs { - pkgPaths.files = append(pkgPaths.files, pathReplacer.Replace(pkg)) - } - pkgPaths.files = append(pkgPaths.files, base) + pkgPaths := dirHTTPFileSystem{append(pkgs, "./"), base} fileHandler := http.FileServer(pkgPaths).ServeHTTP @@ -127,8 +121,8 @@ userIndexFile, err := runfiles.Runfile(base, pathReplacer.Replace(filepath.Join(pkg, "index.html"))) // In case the potential user index file couldn't be found in the runfiles, - // just continue searching. - if _, statErr := os.Stat(userIndexFile); err != nil || statErr != nil { + // continue searching within other packages. + if err != nil { continue } @@ -175,34 +169,40 @@ // dirHTTPFileSystem implements http.FileSystem by looking in the list of dirs one after each other. type dirHTTPFileSystem struct { - files []string + packageDirs []string base string } func (fs dirHTTPFileSystem) Open(name string) (http.File, error) { - for _, packageName := range fs.files { - filePackageName := filepath.Join(packageName, name) - realFilePath, err := runfiles.Runfile(fs.base, filePackageName) - stat, statErr := os.Stat(realFilePath) + for _, packageName := range fs.packageDirs { + manifestFilePath := filepath.Join(packageName, name) + realFilePath, err := runfiles.Runfile(fs.base, manifestFilePath) - if err != nil || statErr != nil { + if err != nil { // In case the runfile could not be found, we also need to check that the requested // path does not refer to a directory containing an "index.html" file. This can // happen if Bazel runs without runfile symlinks, where only files can be resolved // from the manifest. In that case we dirty check if there is a "index.html" file. - realFilePath, err = runfiles.Runfile(fs.base, filepath.Join(filePackageName, "index.html")) + realFilePath, err = runfiles.Runfile(fs.base, filepath.Join(manifestFilePath, "index.html")) - // Continue searching if the runfile couldn't be found for the request filed. - if _, statErr := os.Stat(realFilePath); err != nil || statErr != nil { + // Continue searching if the runfile couldn't be found for the requested file. + if err != nil { continue } } + stat, err := os.Stat(realFilePath) + if err != nil { + // This should actually never happen because runfiles resolved through the runfile helpers + // should always exist. Just in order to properly handle the error, we add this error handling. + return nil, fmt.Errorf("could not read runfile %s", manifestFilePath) + } + // In case the resolved file resolves to a directory. This can only happen if // Bazel runs with symlinked runfiles (e.g. on MacOS, linux). In that case, we // just look for a index.html in the directory. if stat.IsDir() { - realFilePath, err = runfiles.Runfile(fs.base, filepath.Join(filePackageName, "index.html")) + realFilePath, err = runfiles.Runfile(fs.base, filepath.Join(manifestFilePath, "index.html")) // In case the index.html file of the requested directory couldn't be found, // we just continue searching.
diff --git a/devserver/main.go b/devserver/main.go index 29c224b..7b0a2a0 100644 --- a/devserver/main.go +++ b/devserver/main.go
@@ -8,15 +8,17 @@ "io/ioutil" "net/http" "os" - "path/filepath" "strings" "github.com/bazelbuild/rules_typescript/devserver/concatjs" "github.com/bazelbuild/rules_typescript/devserver/devserver" + "github.com/bazelbuild/rules_typescript/devserver/runfiles" ) var ( port = flag.Int("port", 5432, "server port to listen on") + // The "base" CLI flag is only kept because within Google3 because removing would be a breaking change due to + // ConcatJS and "devserver/devserver.go" still respecting the specified base flag. base = flag.String("base", "", "server base (required, runfiles of the binary)") pkgs = flag.String("packages", "", "root package(s) to serve, comma-separated") manifest = flag.String("manifest", "", "sources manifest (.MF)") @@ -28,23 +30,30 @@ func main() { flag.Parse() - if *base == "" || len(*pkgs) == 0 || (*manifest == "") || (*scriptsManifest == "") { + if len(*pkgs) == 0 || (*manifest == "") || (*scriptsManifest == "") { fmt.Fprintf(os.Stderr, "Required argument not set\n") os.Exit(1) } - if _, err := os.Stat(*base); err != nil { - fmt.Fprintf(os.Stderr, "Cannot read server base %s: %v\n", *base, err) + manifestPath, err := runfiles.Runfile(*base, *scriptsManifest) + + if err != nil { + fmt.Fprintf(os.Stderr, "Failed to find scripts_manifest in runfiles: %v\n", err) os.Exit(1) } - scriptsManifestPath := filepath.Join(*base, *scriptsManifest) - scriptFiles, err := manifestFiles(scriptsManifestPath) + scriptFiles, err := manifestFiles(manifestPath) if err != nil { fmt.Fprintf(os.Stderr, "Failed to read scripts_manifest: %v\n", err) os.Exit(1) } + if !strings.HasPrefix(*servingPath, "/") { + fmt.Fprintf(os.Stderr, "The specified serving_path does not start with a slash. "+ + "This causes the serving path to not have any effect.\n") + os.Exit(1) + } + preScripts := make([]string, 0, 100) postScripts := make([]string, 0, 1) @@ -76,7 +85,12 @@ // the requirejs script which is added to scriptFiles by the devserver // skylark rule. for _, v := range scriptFiles { - js, err := loadScript(filepath.Join(*base, v)) + runfile, err := runfiles.Runfile(*base, v) + if err != nil { + fmt.Fprintf(os.Stderr, "Could not find runfile %s, got error %s", v, err) + } + + js, err := loadScript(runfile) if err != nil { fmt.Fprintf(os.Stderr, "Failed to read script %s: %v\n", v, err) } else { @@ -90,7 +104,8 @@ postScripts = append(postScripts, fmt.Sprintf("require([\"%s\"]);", *entryModule)) } - http.Handle(*servingPath, concatjs.ServeConcatenatedJS(*manifest, *base, preScripts, postScripts, nil /* realFileSystem */)) + http.Handle(*servingPath, concatjs.ServeConcatenatedJS(*manifest, *base, preScripts, postScripts, + &RunfileFileSystem{})) pkgList := strings.Split(*pkgs, ",") http.HandleFunc("/", devserver.CreateFileHandler(*servingPath, *manifest, pkgList, *base))
diff --git a/devserver/runfile-filesystem.go b/devserver/runfile-filesystem.go index 3f99c2f..090ded6 100644 --- a/devserver/runfile-filesystem.go +++ b/devserver/runfile-filesystem.go
@@ -30,6 +30,6 @@ // ResolvePath resolves the specified path within a given root using Bazel's runfile resolution. // This is necessary because on Windows, runfiles are not symlinked and need to be // resolved using the runfile manifest file. -func (fs *RunfileFileSystem) ResolvePath(root string, file string) (string, error) { - return runfiles.Runfile(root, file) +func (fs *RunfileFileSystem) ResolvePath(root string, manifestFilePath string) (string, error) { + return runfiles.Runfile(root, manifestFilePath) }
diff --git a/devserver/runfiles/runfiles.go b/devserver/runfiles/runfiles.go index 1d03448..cff9acc 100644 --- a/devserver/runfiles/runfiles.go +++ b/devserver/runfiles/runfiles.go
@@ -4,7 +4,7 @@ import "github.com/bazelbuild/rules_go/go/tools/bazel" // Runfile returns the base directory to the bazel runfiles -func Runfile(_ string, path string) (string, error) { - return bazel.Runfile(path) +func Runfile(_base string, manifestPath string) (string, error) { + return bazel.Runfile(manifestPath) }
diff --git a/internal/devserver/BUILD b/internal/devserver/BUILD index d6e9733..2dfa86b 100644 --- a/internal/devserver/BUILD +++ b/internal/devserver/BUILD
@@ -19,6 +19,7 @@ ]) exports_files([ + "launcher_template.sh", # Exported to be consumed for generating skydoc. "ts_devserver.bzl", ])
diff --git a/internal/devserver/launcher_template.sh b/internal/devserver/launcher_template.sh new file mode 100644 index 0000000..13722a9 --- /dev/null +++ b/internal/devserver/launcher_template.sh
@@ -0,0 +1,59 @@ +#!/bin/bash + +# Copyright 2018 The Bazel Authors. All rights reserved. +# +# Licensed under the Apache License, Version 2.0 (the "License"); +# you may not use this file except in compliance with the License. +# You may obtain a copy of the License at +# +# http://www.apache.org/licenses/LICENSE-2.0 +# +# Unless required by applicable law or agreed to in writing, software +# distributed under the License is distributed on an "AS IS" BASIS, +# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. +# See the License for the specific language governing permissions and +# limitations under the License. + +set -e + +# --- begin runfiles.bash initialization --- +# Source the runfiles library: +# https://github.com/bazelbuild/bazel/blob/master/tools/bash/runfiles/runfiles.bash +# The runfiles library defines rlocation, which is a platform independent function +# used to lookup the runfiles locations. This code snippet is needed at the top +# of scripts that use rlocation to lookup the location of runfiles.bash and source it +if [[ ! -d "${RUNFILES_DIR:-/dev/null}" && ! -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + if [[ -f "$0.runfiles_manifest" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles_manifest" + elif [[ -f "$0.runfiles/MANIFEST" ]]; then + export RUNFILES_MANIFEST_FILE="$0.runfiles/MANIFEST" + elif [[ -f "$0.runfiles/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + export RUNFILES_DIR="$0.runfiles" + fi +fi +if [[ -f "${RUNFILES_DIR:-/dev/null}/bazel_tools/tools/bash/runfiles/runfiles.bash" ]]; then + source "${RUNFILES_DIR}/bazel_tools/tools/bash/runfiles/runfiles.bash" +elif [[ -f "${RUNFILES_MANIFEST_FILE:-/dev/null}" ]]; then + source "$(grep -m1 "^bazel_tools/tools/bash/runfiles/runfiles.bash " \ + "$RUNFILES_MANIFEST_FILE" | cut -d ' ' -f 2-)" +else + echo >&2 "ERROR: cannot find @bazel_tools//tools/bash/runfiles:runfiles.bash" + exit 1 +fi +# --- end runfiles.bash initialization --- + + +readonly main=$(rlocation "TEMPLATED_main") +readonly manifest=$(rlocation "TEMPLATED_manifest") +readonly scripts_manifest=$(rlocation "TEMPLATED_scripts_manifest") + +# Workaround for https://github.com/bazelbuild/bazel/issues/6764 +# If this issue is incorporated into Bazel, the workaround here should be removed. +MSYS2_ARG_CONV_EXCL="*" "${main}" \ + -packages=TEMPLATED_packages \ + -serving_path=TEMPLATED_serving_path \ + -entry_module=TEMPLATED_entry_module \ + -port=TEMPLATED_port \ + -manifest="${manifest}" \ + -scripts_manifest="${scripts_manifest}" \ + "$@"
diff --git a/internal/devserver/ts_devserver.bzl b/internal/devserver/ts_devserver.bzl index 3fd9519..fe377ec 100644 --- a/internal/devserver/ts_devserver.bzl +++ b/internal/devserver/ts_devserver.bzl
@@ -24,6 +24,14 @@ "html_asset_inject", ) +# Helper function to convert a short path to a path that is +# found in the MANIFEST file. +def _short_path_to_manifest_path(ctx, short_path): + if short_path.startswith("../"): + return short_path[3:] + else: + return ctx.workspace_name + "/" + short_path + def _ts_devserver(ctx): files = depset() for d in ctx.attr.deps: @@ -52,7 +60,7 @@ amd_names_shim = ctx.actions.declare_file( "_%s.amd_names_shim.js" % ctx.label.name, - sibling = ctx.outputs.executable, + sibling = ctx.outputs.script, ) write_amd_names_shim(ctx.actions, amd_names_shim, ctx.attr.bootstrap) @@ -76,6 +84,7 @@ ] devserver_runfiles += ctx.files.static_files devserver_runfiles += script_files + devserver_runfiles += ctx.files._bash_runfile_helpers if ctx.file.index_html: injected_index = ctx.actions.declare_file("index.html") @@ -96,37 +105,24 @@ ) devserver_runfiles += [injected_index] - serving_arg = "" - if ctx.attr.serving_path: - serving_arg = "-serving_path=%s" % ctx.attr.serving_path - packages = depset(["/".join([workspace_name, ctx.label.package])] + ctx.attr.additional_root_paths) - # FIXME: more bash dependencies makes Windows support harder - ctx.actions.write( - output = ctx.outputs.executable, + ctx.actions.expand_template( + template = ctx.file._launcher_template, + output = ctx.outputs.script, + substitutions = { + "TEMPLATED_entry_module": ctx.attr.entry_module, + "TEMPLATED_main": _short_path_to_manifest_path(ctx, ctx.executable._devserver.short_path), + "TEMPLATED_manifest": _short_path_to_manifest_path(ctx, ctx.outputs.manifest.short_path), + "TEMPLATED_packages": ",".join(packages.to_list()), + "TEMPLATED_port": str(ctx.attr.port), + "TEMPLATED_scripts_manifest": _short_path_to_manifest_path(ctx, ctx.outputs.scripts_manifest.short_path), + "TEMPLATED_serving_path": ctx.attr.serving_path if ctx.attr.serving_path else "", + "TEMPLATED_workspace": workspace_name, + }, is_executable = True, - content = """#!/bin/sh -RUNFILES="$PWD/.." -{main} {serving_arg} \ - -base="$RUNFILES" \ - -packages={packages} \ - -manifest={workspace}/{manifest} \ - -scripts_manifest={workspace}/{scripts_manifest} \ - -entry_module={entry_module} \ - -port={port} \ - "$@" -""".format( - main = ctx.executable._devserver.short_path, - serving_arg = serving_arg, - workspace = workspace_name, - packages = ",".join(packages.to_list()), - manifest = ctx.outputs.manifest.short_path, - scripts_manifest = ctx.outputs.scripts_manifest.short_path, - entry_module = ctx.attr.entry_module, - port = str(ctx.attr.port), - ), ) + return [DefaultInfo( runfiles = ctx.runfiles( files = devserver_runfiles, @@ -191,6 +187,7 @@ allow_files = True, aspects = [sources_aspect], ), + "_bash_runfile_helpers": attr.label(default = Label("@bazel_tools//tools/bash/runfiles")), "_devserver": attr.label( # For local development in rules_typescript, we build the devserver from sources. # This requires that we have the go toolchain available. @@ -206,33 +203,52 @@ executable = True, cfg = "host", ), + "_launcher_template": attr.label(allow_single_file = True, default = Label("//internal/devserver:launcher_template.sh")), "_requirejs_script": attr.label(allow_single_file = True, default = Label("@build_bazel_rules_typescript_devserver_deps//node_modules/requirejs:require.js")), }, outputs = { "manifest": "%{name}.MF", + "script": "%{name}.sh", "scripts_manifest": "scripts_%{name}.MF", }, - executable = True, ) """ts_devserver is a simple development server intended for a quick "getting started" experience. Additional documentation at https://github.com/alexeagle/angular-bazel-example/wiki/Running-a-devserver-under-Bazel """ -def ts_devserver_macro(tags = [], **kwargs): - """ibazel wrapper for `ts_devserver` +def ts_devserver_macro(name, data = [], args = [], visibility = None, tags = [], testonly = 0, **kwargs): + """Macro for creating a `ts_devserver` - This macro re-exposes the `ts_devserver` rule with some extra tags so that - it behaves correctly under ibazel. - + This macro re-exposes a `sh_binary` and `ts_devserver` target that can run the + actual devserver implementation. + The `ts_devserver` rule is just responsible for generating a launcher script + that runs the Go devserver implementation. The `sh_binary` is the primary + target that matches the specified "name" and executes the generated bash + launcher script. This is re-exported in `//:defs.bzl` as `ts_devserver` so if you load the rule from there, you actually get this macro. - Args: - tags: standard Bazel tags, this macro adds a couple for ibazel + name: Name of the devserver target + data: Runtime dependencies for the devserver + args: Command line arguments that will be passed to the devserver Go implementation + visibility: Visibility of the devserver targets + tags: Standard Bazel tags, this macro adds a couple for ibazel + testonly: Whether the devserver should only run in `bazel test` **kwargs: passed through to `ts_devserver` """ ts_devserver( + name = "%s_launcher" % name, + data = data + ["@bazel_tools//tools/bash/runfiles"], + testonly = testonly, + visibility = ["//visibility:private"], + tags = tags, + **kwargs + ) + + native.sh_binary( + name = name, + args = args, # Users don't need to know that these tags are required to run under ibazel tags = tags + [ # Tell ibazel not to restart the devserver when its deps change. @@ -241,5 +257,8 @@ # this program. "ibazel_live_reload", ], - **kwargs + srcs = ["%s_launcher.sh" % name], + data = [":%s_launcher" % name], + testonly = testonly, + visibility = visibility, )