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(builtin): properly parse status file value containing spaces #2615

Conversation

josephperrott
Copy link
Collaborator

Based on Bazel's documentation, values provided through the workspace_status_command can contain
whitespace on the same line the key is defined on:

The key names can be anything but they may only use upper case letters and underscores. The
first space after the key name separates it from the value. The value is the rest of the line
(including additional whitespaces).

The parseStatusFile utility function is updated to rely on a regex for performing this parsing to
match the described behavior.

@alexeagle
Copy link
Collaborator

failure looks legit

ERROR: /home/circleci/rules_nodejs/internal/pkg_npm/test/BUILD.bazel:82:8: Executing genrule //internal/pkg_npm/test:gen_test_pkg_pack failed: (Exit 1): bash failed: error executing command /bin/bash -c ... (remaining 1 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox bash failed: error executing command /bin/bash -c ... (remaining 1 argument(s) skipped)

Use --sandbox_debug to see verbose messages from the sandbox
npm ERR! Invalid version: "{BUILD_SCM_VERSION}"

Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

This occurs in a couple other places, do you mind fixing those too?

internal/pkg_web/assembler.js
a few rollup.config.js files
install.md

Based on Bazel's documentation, values provided through the workspace_status_command can contain
whitespace on the same line the key is defined on:

> The key names can be anything but they may only use upper case letters and underscores. The
> first space after the key name separates it from the value. The value is the rest of the line
> (including additional whitespaces).

The `parseStatusFile` utility function is updated to rely on a regex for performing this parsing to
match the described behavior.
@josephperrott josephperrott force-pushed the correct-parse-status-file-function branch from 048d975 to 76e7bc1 Compare April 19, 2021 19:57
@josephperrott josephperrott requested a review from jbedard as a code owner April 19, 2021 19:57
@josephperrott josephperrott requested a review from alexeagle April 19, 2021 20:22
Copy link
Collaborator

@alexeagle alexeagle left a comment

Choose a reason for hiding this comment

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

looks like we ought to come back with a refactoring later that makes this function available through our built-in somehow. thanks for fixing!

@alexeagle alexeagle merged commit 406dcb5 into bazel-contrib:stable Apr 19, 2021
@josephperrott josephperrott deleted the correct-parse-status-file-function branch April 19, 2021 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants