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.

Sign up for GitHub

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

Jump to bottom

refactor(builtin): cleanup and optimizations in nodejs_binary & linker #1799

Merged
merged 1 commit into from
Apr 7, 2020
Merged

refactor(builtin): cleanup and optimizations in nodejs_binary & linker #1799

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 3 additions & 1 deletion internal/linker/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -4,7 +4,9 @@ load("@npm_bazel_typescript//:index.from_src.bzl", "checked_in_ts_library")

# We can't bootstrap the ts_library rule using the linker itself,
# because the implementation of ts_library depends on the linker so that would be a cycle.
# So we compile it to JS and check in the result as index.js
# So we compile it to JS and check in the result as index.js.
# To update index.js run:
# bazel run //internal/linker:linker_lib_check_compiled.accept
checked_in_ts_library(
name = "linker_lib",
srcs = ["link_node_modules.ts"],
Expand Down
31 changes: 11 additions & 20 deletions internal/linker/index.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -172,10 +172,9 @@ Include as much of the build output as you can without disclosing anything confi
If you want to test runfiles manifest behavior, add
--spawn_strategy=standalone to the command line.`);
}
const wksp = env['BAZEL_WORKSPACE'];
if (!!wksp) {
this.workspace = wksp;
}
// Bazel starts actions with pwd=execroot/my_wksp
this.workspaceDir = path.resolve('.');
this.workspace = path.basename(this.workspaceDir);
// If target is from an external workspace such as @npm//rollup/bin:rollup
// resolvePackageRelative is not supported since package is in an external
// workspace.
Expand Down Expand Up @@ -238,17 +237,11 @@ Include as much of the build output as you can without disclosing anything confi
throw new Error(`could not resolve modulePath ${modulePath}`);
}
resolveWorkspaceRelative(modulePath) {
if (!this.workspace) {
throw new Error('workspace could not be determined from the environment');
}
return this.resolve(path.posix.join(this.workspace, modulePath));
}
resolvePackageRelative(modulePath) {
if (!this.workspace) {
throw new Error('workspace could not be determined from the environment');
}
if (!this.package) {
throw new Error('package could not be determined from the environment');
throw new Error('package could not be determined from the environment; make sure BAZEL_TARGET is set');
}
return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
}
Expand Down Expand Up @@ -432,17 +425,15 @@ Include as much of the build output as you can without disclosing anything confi
modules = modules || {};
log_verbose('manifest file', modulesManifest);
log_verbose('manifest contents', JSON.stringify({ workspace, bin, root, modules }, null, 2));
// Bazel starts actions with pwd=execroot/my_wksp
const workspaceDir = path.resolve('.');
// resolveRoot will change the cwd when under runfiles
// NB: resolveRoot will change the cwd when under runfiles to the runfiles root
const rootDir = yield resolveRoot(root, runfiles);
log_verbose('resolved root', root, 'to', rootDir);
log_verbose('resolved node_modules root', root, 'to', rootDir);
log_verbose('cwd', process.cwd());
// Create the $pwd/node_modules directory that node will resolve from
// Create the node_modules symlink to the node_modules root that node will resolve from
yield symlink(rootDir, 'node_modules');
// Change directory to the node_modules root directory so that all subsequent
// symlinks will be created under node_modules
process.chdir(rootDir);
// Symlinks to packages need to reach back to the workspace/runfiles directory
const workspaceAbs = path.resolve(workspaceDir);
function linkModules(m) {
return __awaiter(this, void 0, void 0, function* () {
// ensure the parent directory exist
Expand All @@ -454,7 +445,7 @@ Include as much of the build output as you can without disclosing anything confi
switch (root) {
case 'execroot':
if (runfiles.execroot) {
target = path.posix.join(workspaceAbs, modulePath);
target = path.posix.join(runfiles.workspaceDir, modulePath);
}
else {
// If under runfiles, convert from execroot path to runfiles path.
Expand All @@ -470,7 +461,7 @@ Include as much of the build output as you can without disclosing anything confi
if (runfilesPath.startsWith(externalPrefix)) {
runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`;
}
target = path.posix.join(workspaceAbs, runfilesPath);
target = path.posix.join(runfiles.workspaceDir, runfilesPath);
}
break;
case 'runfiles':
Expand Down
41 changes: 15 additions & 26 deletions internal/linker/link_node_modules.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -131,10 +131,8 @@ export class Runfiles {
manifest: Map<string, string>|undefined;
dir: string|undefined;
execroot: boolean;
/**
* If the environment gives us enough hints, we can know the workspace name
*/
workspace: string|undefined;
workspace: string;
workspaceDir: string;
/**
* If the environment gives us enough hints, we can know the package path
*/
Expand Down Expand Up @@ -166,10 +164,9 @@ export class Runfiles {
If you want to test runfiles manifest behavior, add
--spawn_strategy=standalone to the command line.`);
}
const wksp = env['BAZEL_WORKSPACE'];
if (!!wksp) {
this.workspace = wksp;
}
// Bazel starts actions with pwd=execroot/my_wksp
this.workspaceDir = path.resolve('.');
this.workspace = path.basename(this.workspaceDir);
// If target is from an external workspace such as @npm//rollup/bin:rollup
// resolvePackageRelative is not supported since package is in an external
// workspace.
Expand Down Expand Up @@ -239,18 +236,13 @@ export class Runfiles {
}

resolveWorkspaceRelative(modulePath: string) {
if (!this.workspace) {
throw new Error('workspace could not be determined from the environment');
}
return this.resolve(path.posix.join(this.workspace, modulePath));
}

resolvePackageRelative(modulePath: string) {
if (!this.workspace) {
throw new Error('workspace could not be determined from the environment');
}
if (!this.package) {
throw new Error('package could not be determined from the environment');
throw new Error(
'package could not be determined from the environment; make sure BAZEL_TARGET is set');
}
return this.resolve(path.posix.join(this.workspace, this.package, modulePath));
}
Expand Down Expand Up @@ -493,20 +485,17 @@ export async function main(args: string[], runfiles: Runfiles) {
log_verbose('manifest file', modulesManifest);
log_verbose('manifest contents', JSON.stringify({workspace, bin, root, modules}, null, 2));

// Bazel starts actions with pwd=execroot/my_wksp
const workspaceDir = path.resolve('.');

// resolveRoot will change the cwd when under runfiles
// NB: resolveRoot will change the cwd when under runfiles to the runfiles root
const rootDir = await resolveRoot(root, runfiles);
log_verbose('resolved root', root, 'to', rootDir);
log_verbose('resolved node_modules root', root, 'to', rootDir);
log_verbose('cwd', process.cwd());

// Create the $pwd/node_modules directory that node will resolve from
// Create the node_modules symlink to the node_modules root that node will resolve from
await symlink(rootDir, 'node_modules');
process.chdir(rootDir);

// Symlinks to packages need to reach back to the workspace/runfiles directory
const workspaceAbs = path.resolve(workspaceDir);
// Change directory to the node_modules root directory so that all subsequent
// symlinks will be created under node_modules
process.chdir(rootDir);

async function linkModules(m: LinkerTreeElement) {
// ensure the parent directory exist
Expand All @@ -519,7 +508,7 @@ export async function main(args: string[], runfiles: Runfiles) {
switch (root) {
case 'execroot':
if (runfiles.execroot) {
target = path.posix.join(workspaceAbs, modulePath);
target = path.posix.join(runfiles.workspaceDir, modulePath);
} else {
// If under runfiles, convert from execroot path to runfiles path.
// First strip the bin portion if it exists:
Expand All @@ -533,7 +522,7 @@ export async function main(args: string[], runfiles: Runfiles) {
if (runfilesPath.startsWith(externalPrefix)) {
runfilesPath = `../${runfilesPath.slice(externalPrefix.length)}`;
}
target = path.posix.join(workspaceAbs, runfilesPath);
target = path.posix.join(runfiles.workspaceDir, runfilesPath);
}
break;
case 'runfiles':
Expand Down
1 change: 0 additions & 1 deletion internal/node/node.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -179,7 +179,6 @@ def _nodejs_binary_impl(ctx):
_write_loader_script(ctx)

env_vars = "export BAZEL_TARGET=%s\n" % ctx.label
env_vars += "export BAZEL_WORKSPACE=%s\n" % ctx.workspace_name
Copy link
Collaborator

Choose a reason for hiding this comment

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

hmm, we didn't expect this anywhere else? looks like a breaking change to remove?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

its undocumented & internal use only, so I wouldn't consider it a breaking change

for k in ctx.attr.configuration_env_vars + ctx.attr.default_env_vars:
# Check ctx.var first & if env var not in there then check
# ctx.configuration.default_shell_env. The former will contain values from --define=FOO=BAR
Expand Down
9 changes: 6 additions & 3 deletions third_party/github.com/bazelbuild/bazel/tools/bash/runfiles/runfiles.bash
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -135,9 +135,12 @@ function rlocation() {
return 1
else
# --- begin rules_nodejs custom code ---
# Normalize ${BAZEL_WORKSPACE}/$1.
# Bazel always sets the PWD to execroot/my_wksp so we derive the workspace
# from the PWD.
export bazel_workspace=$(basename $PWD)
# Normalize ${bazel_workspace}/$1.
# If $1 is a $(rootpath) this will convert it to the runfiles manifest path
readonly from_rootpath=$(normpath ${BAZEL_WORKSPACE:-/dev/null}/$1)
readonly from_rootpath=$(normpath ${bazel_workspace}/$1)
# --- end rules_nodejs custom code ---
if [[ -e "${RUNFILES_DIR:-/dev/null}/$1" ]]; then
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
Expand All @@ -148,7 +151,7 @@ function rlocation() {
# If $1 is a rootpath then check if the converted rootpath to runfiles manifest path file is found
elif [[ -e "${RUNFILES_DIR:-/dev/null}/${from_rootpath}" ]]; then
if [[ "${RUNFILES_LIB_DEBUG:-}" == 1 ]]; then
echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/BAZEL_WORKSPACE ($RUNFILES_DIR/$BAZEL_WORKSPACE), return"
echo >&2 "INFO[runfiles.bash]: rlocation($1): found under RUNFILES_DIR/WKSP ($RUNFILES_DIR/$bazel_workspace), return"
fi
echo "${RUNFILES_DIR}/${from_rootpath}"
# --- end rules_nodejs custom code ---
Expand Down