Skip to content
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

fix: change all cfg=host and cfg=target executable attributes to cfg=exec #3215

Merged
merged 1 commit into from Jan 11, 2022
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
7 changes: 5 additions & 2 deletions examples/angular/src/tsconfig.json
Expand Up @@ -25,13 +25,16 @@

// Add all possible bazel-out directories
// See https://github.com/microsoft/TypeScript/issues/37257
"../bazel-out/darwin_arm64-fastbuild/bin/src",
"../bazel-out/darwin-dbg/bin/src",
"../bazel-out/darwin-fastbuild/bin/src",
"../bazel-out/darwin_arm64-fastbuild/bin/src",
"../bazel-out/darwin-opt-exec-2B5CBBC6/bin/src",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

do you know what the 2B5CBBC6 is based on? I hope it's a stable checksum that will match on another machine.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

John Cater says "the hex tag is just a hash of the execution platform" (bazelbuild/bazel#12403 (comment)) so it should be stable. Guessing it is a hash of "host" if the exec platform is the host since it is stable between k8, darwin & windows in these paths

"../bazel-out/k8-dbg/bin/src",
"../bazel-out/k8-fastbuild/bin/src",
"../bazel-out/k8-opt-exec-2B5CBBC6/bin/src",
"../bazel-out/x64_windows-dbg/bin/src",
"../bazel-out/x64_windows-fastbuild/bin/src"
"../bazel-out/x64_windows-fastbuild/bin/src",
"../bazel-out/x64_windows-opt-exec-2B5CBBC6/bin/src"
]
}
}
2 changes: 1 addition & 1 deletion examples/angular/tools/karma.bzl
Expand Up @@ -70,7 +70,7 @@ _generate_karma_config = rule(
# https://github.com/bazelbuild/rules_nodejs/blob/3.3.0/packages/concatjs/web_test/karma_web_test.bzl#L88-L91
"_conf_tmpl": attr.label(
doc = """the karma config template""",
cfg = "host",
cfg = "exec",
allow_single_file = True,
default = Label("//tools:karma.conf.js"),
),
Expand Down
2 changes: 1 addition & 1 deletion examples/parcel/parcel.bzl
Expand Up @@ -57,7 +57,7 @@ parcel = rule(
# This default assumes that users name their install "npm"
default = Label("@npm//parcel-bundler/bin:parcel"),
executable = True,
cfg = "host",
cfg = "exec",
),
"srcs": attr.label_list(allow_files = True),
},
Expand Down
2 changes: 1 addition & 1 deletion examples/worker/uses_workers.bzl
Expand Up @@ -34,7 +34,7 @@ work = rule(
"tool": attr.label(
default = Label("//:tool"),
executable = True,
cfg = "host",
cfg = "exec",
),
},
)
Expand Up @@ -244,7 +244,7 @@ is published on GitHub.
),
"_test_runner": attr.label(
executable = True,
cfg = "host",
cfg = "exec",
default = Label("@build_bazel_rules_nodejs//internal/bazel_integration_test:test_runner"),
),
}
Expand Down
2 changes: 1 addition & 1 deletion internal/linker/test/integration/rule.bzl
Expand Up @@ -20,5 +20,5 @@ linked = rule(_linked, attrs = {
aspects = [module_mappings_aspect],
),
"out": attr.output(),
"program": attr.label(executable = True, cfg = "host", mandatory = True),
"program": attr.label(executable = True, cfg = "exec", mandatory = True),
})
2 changes: 1 addition & 1 deletion internal/linker/test/no_npm_deps/no_npm_deps.bzl
Expand Up @@ -11,7 +11,7 @@ _ATTRS = {
),
"terser": attr.label(
executable = True,
cfg = "host",
cfg = "exec",
default = Label("@npm//terser/bin:terser"),
),
}
Expand Down
2 changes: 1 addition & 1 deletion internal/node/node.bzl
Expand Up @@ -665,7 +665,7 @@ nodejs_test_kwargs = dict(
"_lcov_merger": attr.label(
executable = True,
default = Label("@build_bazel_rules_nodejs//internal/coverage:lcov_merger_sh"),
cfg = "target",
cfg = "exec",
),
}),
doc = """
Expand Down
2 changes: 1 addition & 1 deletion internal/node/npm_package_bin.bzl
Expand Up @@ -21,7 +21,7 @@ _ATTRS = {
"stdout": attr.output(),
"tool": attr.label(
executable = True,
cfg = "host",
cfg = "exec",
mandatory = True,
),
"stamp": STAMP_ATTR,
Expand Down
2 changes: 1 addition & 1 deletion internal/npm_install/npm_umd_bundle.bzl
Expand Up @@ -114,7 +114,7 @@ This target would be then be used instead of the generated `@npm//typeorm:typeor
),
"_browserify_wrapped": attr.label(
executable = True,
cfg = "host",
cfg = "exec",
default = Label("@build_bazel_rules_nodejs//internal/npm_install:browserify-wrapped"),
),
}
Expand Down
2 changes: 1 addition & 1 deletion internal/pkg_web/pkg_web.bzl
Expand Up @@ -38,7 +38,7 @@ See the section on stamping in the README.""",
"_assembler": attr.label(
default = "@build_bazel_rules_nodejs//internal/pkg_web:assembler",
executable = True,
cfg = "host",
cfg = "exec",
),
}

Expand Down
6 changes: 3 additions & 3 deletions internal/providers/test/run_node_test.bzl
Expand Up @@ -75,17 +75,17 @@ js_write_file = rule(
"content": attr.string(),
"_clone": attr.label(
default = Label("//internal/providers/test:cloner_bin"),
cfg = "host",
cfg = "exec",
executable = True,
),
"_writer": attr.label(
default = Label("//internal/providers/test:writer_bin"),
cfg = "host",
cfg = "exec",
executable = True,
),
"_writer2": attr.label(
default = Label("//internal/providers/test:writer_bin2"),
cfg = "host",
cfg = "exec",
executable = True,
),
},
Expand Down
4 changes: 2 additions & 2 deletions packages/concatjs/devserver/concatjs_devserver.bzl
Expand Up @@ -152,14 +152,14 @@ concatjs_devserver = rule(
Defaults to precompiled go binary setup by @bazel/typescript npm package""",
default = Label("//packages/concatjs/devserver"),
executable = True,
cfg = "host",
cfg = "exec",
),
"devserver_host": attr.label(
doc = """Go based devserver executable for the host platform.
Defaults to precompiled go binary setup by @bazel/typescript npm package""",
default = Label("//packages/concatjs/devserver"),
executable = True,
cfg = "host",
cfg = "exec",
),
"entry_module": attr.string(
doc = """The `entry_module` should be the AMD module name of the entry module such as `"__main__/src/index".`
Expand Down
2 changes: 1 addition & 1 deletion packages/concatjs/internal/build_defs.bzl
Expand Up @@ -407,7 +407,7 @@ then it needs to be a data dependency of `tsc_wrapped` so that it can be loaded
default = Label(_DEFAULT_COMPILER),
allow_files = True,
executable = True,
cfg = "host",
cfg = "exec",
),
"deps": attr.label_list(
aspects = DEPS_ASPECTS + [node_modules_aspect],
Expand Down
2 changes: 1 addition & 1 deletion packages/concatjs/test/some_module/run_node_test.bzl
Expand Up @@ -23,7 +23,7 @@ ts_write_file = rule(
"content": attr.string(),
"_writer": attr.label(
default = Label("//packages/concatjs/test/some_module:writer_bin"),
cfg = "host",
cfg = "exec",
executable = True,
),
},
Expand Down
2 changes: 1 addition & 1 deletion packages/concatjs/web_test/karma_web_test.bzl
Expand Up @@ -65,7 +65,7 @@ KARMA_WEB_TEST_ATTRS = {
# NB: replaced during pkg_npm with "@npm//karma/bin:karma"
default = "//packages/concatjs/web_test:karma_bin",
executable = True,
cfg = "target",
cfg = "exec",
allow_files = True,
),
"runtime_deps": attr.label_list(
Expand Down
4 changes: 2 additions & 2 deletions packages/protractor/protractor_web_test.bzl
Expand Up @@ -213,13 +213,13 @@ _protractor_web_test = rule(
"protractor": attr.label(
doc = "Protractor executable target",
executable = True,
cfg = "target",
cfg = "exec",
allow_files = True,
),
"server": attr.label(
doc = "Optional server executable target",
executable = True,
cfg = "target",
cfg = "exec",
allow_files = True,
),
"srcs": attr.label_list(
Expand Down
4 changes: 2 additions & 2 deletions packages/rollup/rollup_bundle.bzl
Expand Up @@ -118,7 +118,7 @@ Otherwise, the outputs are assumed to be a single file.
"rollup_bin": attr.label(
doc = "Target that executes the rollup binary",
executable = True,
cfg = "host",
cfg = "exec",
default = (
# BEGIN-INTERNAL
"@npm" +
Expand All @@ -129,7 +129,7 @@ Otherwise, the outputs are assumed to be a single file.
"rollup_worker_bin": attr.label(
doc = "Internal use only",
executable = True,
cfg = "host",
cfg = "exec",
default = "//packages/rollup/bin:rollup-worker",
),
"silent": attr.bool(
Expand Down
45 changes: 44 additions & 1 deletion packages/terser/index.js
Expand Up @@ -26,6 +26,45 @@ function isDirectory(input) {
return fs.lstatSync(path.join(process.cwd(), input)).isDirectory();
}

// Returns a single quotes version of str
function singleQuotes(str) {
return `'${str.replace(/'/g, '').replace(/"/g, '')}'`;
}

// Ensures that args are well formed.
// Work-around for an issue on Windows when exec bin path is not quoted.
// In --source-map, base=bazel-out/x64_windows-opt-exec-2B5CBBC6/bin must
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

interesting we didn't have this problem before, we had hyphens in the path already right?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah. I was puzzled. The extra -exec-2B5CBBC6 broke it when there were no quotes 🤷 Something fishy upstream

// be quoted such as base='bazel-out/x64_windows-opt-exec-2B5CBBC6/bin' pr
// terser fails with
// ERROR: `includeSources,base=bazel-out/x64_windows-opt-exec-2B5CBBC6/bin,content=inline,url=bundle.min.js.map` is not a supported option
function fixArgs(args) {
const sourceMapIndex = args.indexOf('--source-map');
if (sourceMapIndex === -1) {
return args;
}
let sourceMapOptions = args[sourceMapIndex + 1].split(',');
sourceMapOptions = sourceMapOptions.map(o => {
const s = o.split('=');
if (s.length == 1) {
return o;
}
switch (s[0]) {
case 'base':
case 'content':
case 'url':
return `${s[0]}=${singleQuotes(s[1])}`;
default:
return o;
}
});

return [
...args.slice(0, sourceMapIndex + 1),
sourceMapOptions.join(','),
...args.slice(sourceMapIndex + 2),
];
}

/**
* Replaces directory url with the outputFile name in the url option of source-map argument
*/
Expand All @@ -35,8 +74,10 @@ function directoryArgs(residualArgs, inputFile, outputFile) {
return residualArgs;
}

let sourceMapOptions = residualArgs[sourceMapIndex + 1].split(',');

// set the correct sourcemap url for this output file
let sourceMapOptions = residualArgs[sourceMapIndex + 1].split(',').map(
sourceMapOptions = sourceMapOptions.map(
o => o.startsWith('url=') ? `url='${path.basename(outputFile)}.map'` : o);

// if an input .map file exists then set the correct sourcemap content option
Expand Down Expand Up @@ -143,6 +184,8 @@ function spawn(cmd, args) {
}

function main() {
process.argv = fixArgs(process.argv)

// Peek at the arguments to find any directories declared as inputs
let argv = process.argv.slice(2);
// terser_minified.bzl always passes the inputs first,
Expand Down
4 changes: 2 additions & 2 deletions packages/terser/terser_minified.bzl
Expand Up @@ -104,7 +104,7 @@ If you want to do this, you can pass a filegroup here.""",
doc = "An executable target that runs Terser",
default = Label("//packages/terser/bin:terser"),
executable = True,
cfg = "host",
cfg = "exec",
),
}

Expand Down Expand Up @@ -149,7 +149,7 @@ def _terser(ctx):
if ctx.attr.sourcemap:
# Source mapping options are comma-packed into one argv
# see https://github.com/terser-js/terser#command-line-usage
source_map_opts = ["includeSources", "base=" + ctx.bin_dir.path]
source_map_opts = ["includeSources", "base='%s'" % ctx.bin_dir.path]

if len(sourcemaps) == 0:
source_map_opts.append("content=inline")
Expand Down
2 changes: 1 addition & 1 deletion packages/typescript/internal/ts_project.bzl
Expand Up @@ -35,7 +35,7 @@ _ATTRS = {
# that compiler might allow more sources than tsc does.
"srcs": attr.label_list(allow_files = True, mandatory = True),
"supports_workers": attr.bool(default = False),
"tsc": attr.label(default = Label(_DEFAULT_TSC), executable = True, cfg = "host"),
"tsc": attr.label(default = Label(_DEFAULT_TSC), executable = True, cfg = "exec"),
"transpile": attr.bool(doc = "whether tsc should be used to produce .js outputs"),
"tsconfig": attr.label(mandatory = True, allow_single_file = [".json"]),
}
Expand Down
2 changes: 1 addition & 1 deletion packages/typescript/internal/validate_options.bzl
Expand Up @@ -68,7 +68,7 @@ validate_options = rule(
"target": attr.string(),
"ts_build_info_file": attr.string(),
"tsconfig": attr.label(mandatory = True, allow_single_file = [".json"]),
"validator": attr.label(default = Label("//packages/typescript/bin:ts_project_options_validator"), executable = True, cfg = "host"),
"validator": attr.label(default = Label("//packages/typescript/bin:ts_project_options_validator"), executable = True, cfg = "exec"),
},
)

Expand Down