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: remove npm_translate_lock(use_starlark_yaml_parser) #1658

Merged
merged 1 commit into from
Apr 19, 2024
Merged

refactor: remove npm_translate_lock(use_starlark_yaml_parser) #1658

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
3 changes: 1 addition & 2 deletions docs/npm_translate_lock.md
View file
Open in desktop

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

1 change: 0 additions & 1 deletion e2e/npm_translate_lock/WORKSPACE
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,6 @@ npm_translate_lock(
npmrc = "//:.npmrc",
pnpm_lock = "//:pnpm-lock.yaml",
update_pnpm_lock = True,
use_starlark_yaml_parser = True, # test coverage for this flag
verify_node_modules_ignored = "//:.bazelignore",
)

Expand Down
1 change: 0 additions & 1 deletion e2e/webpack_devserver/MODULE.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -12,7 +12,6 @@ npm.npm_translate_lock(
name = "npm",
npmrc = "//:.npmrc",
pnpm_lock = "//:pnpm-lock.yaml",
use_starlark_yaml_parser = True, # test coverage for this flag
verify_node_modules_ignored = "//:.bazelignore",
)
use_repo(npm, "npm")
Expand Down
26 changes: 12 additions & 14 deletions npm/extensions.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,21 +79,19 @@ def _npm_lock_imports_bzlmod(module_ctx, attr):
lock_packages = {}
lock_patched_dependencies = {}
lock_parse_err = None
if attr.use_starlark_yaml_parser:
lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_yaml(module_ctx.read(attr.pnpm_lock))

is_windows = repo_utils.is_windows(module_ctx)
host_yq = Label("@{}_{}//:yq{}".format(attr.yq_toolchain_prefix, repo_utils.platform(module_ctx), ".exe" if is_windows else ""))
yq_args = [
str(module_ctx.path(host_yq)),
str(module_ctx.path(attr.pnpm_lock)),
"-o=json",
]
result = module_ctx.execute(yq_args)
if result.return_code:
lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
else:
is_windows = repo_utils.is_windows(module_ctx)
host_yq = Label("@{}_{}//:yq{}".format(attr.yq_toolchain_prefix, repo_utils.platform(module_ctx), ".exe" if is_windows else ""))
yq_args = [
str(module_ctx.path(host_yq)),
str(module_ctx.path(attr.pnpm_lock)),
"-o=json",
]
result = module_ctx.execute(yq_args)
if result.return_code:
lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
else:
lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None) # NB: yq will return the string "null" if the yaml file is empty
lock_importers, lock_packages, lock_patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None) # NB: yq will return the string "null" if the yaml file is empty

if lock_parse_err != None:
msg = """
Expand Down
7 changes: 0 additions & 7 deletions npm/private/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -211,7 +211,6 @@ bzl_library(
srcs = ["utils.bzl"],
visibility = ["//npm:__subpackages__"],
deps = [
":yaml",
"@aspect_bazel_lib//lib:paths",
"@aspect_bazel_lib//lib:repo_utils",
"@aspect_bazel_lib//lib:utils",
Expand Down Expand Up @@ -304,9 +303,3 @@ bzl_library(
srcs = ["versions.bzl"],
visibility = ["//npm:__subpackages__"],
)

bzl_library(
name = "yaml",
srcs = ["yaml.bzl"],
visibility = ["//npm:__subpackages__"],
)
28 changes: 0 additions & 28 deletions npm/private/npm_translate_lock.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,6 @@ _ATTRS = {
"root_package": attr.string(default = DEFAULT_ROOT_PACKAGE),
"update_pnpm_lock": attr.bool(),
"use_home_npmrc": attr.bool(),
"use_starlark_yaml_parser": attr.bool(),
"verify_node_modules_ignored": attr.label(),
"verify_patches": attr.label(),
"yarn_lock": attr.label(),
Expand Down Expand Up @@ -186,7 +185,6 @@ def npm_translate_lock(
pnpm_version = LATEST_PNPM_VERSION,
use_pnpm = None,
npm_package_target_name = "{dirname}",
use_starlark_yaml_parser = False,
**kwargs):
"""Repository macro to generate starlark code from a lock file.

Expand Down Expand Up @@ -514,31 +512,6 @@ def npm_translate_lock(

Default: `{dirname}`

use_starlark_yaml_parser: Opt-out of using `yq` to parse the pnpm-lock file which was added
in https://github.com/aspect-build/rules_js/pull/1458 and use the legacy starlark yaml
parser instead.

This opt-out is a return safety in cases where yq is not able to parse the pnpm generated
yaml file. For example, this has been observed to happen due to a line such as the following
in the pnpm generated lock file:

```
resolution: {tarball: https://gitpkg.vercel.app/blockprotocol/blockprotocol/packages/%40blockprotocol/type-system-web?6526c0e}
```

where the `?` character in the `tarball` value causes `yq` to fail with:

```
$ yq pnpm-lock.yaml -o=json
Error: bad file 'pnpm-lock.yaml': yaml: line 7129: did not find expected ',' or '}'
```

If the tarball value is quoted or escaped then yq would accept it but as of this writing, the latest
version of pnpm (8.14.3) does not quote or escape such a value and the latest version of yq (4.40.5)
does not handle it as is.

Possibly related to https://github.com/pnpm/pnpm/issues/5414.

**kwargs: Internal use only
"""

Expand Down Expand Up @@ -632,7 +605,6 @@ def npm_translate_lock(
use_pnpm = use_pnpm,
yq_toolchain_prefix = yq_toolchain_prefix,
npm_package_target_name = npm_package_target_name,
use_starlark_yaml_parser = use_starlark_yaml_parser,
bzlmod = bzlmod,
)

Expand Down
22 changes: 10 additions & 12 deletions npm/private/npm_translate_lock_state.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -504,19 +504,17 @@ def _load_lockfile(priv, rctx, label_store):
packages = {}
patched_dependencies = {}
lock_parse_err = None
if rctx.attr.use_starlark_yaml_parser:
importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_yaml(rctx.read(label_store.path("pnpm_lock")))

yq_args = [
str(label_store.path("host_yq")),
str(label_store.path("pnpm_lock")),
"-o=json",
]
result = rctx.execute(yq_args)
if result.return_code:
lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
else:
yq_args = [
str(label_store.path("host_yq")),
str(label_store.path("pnpm_lock")),
"-o=json",
]
result = rctx.execute(yq_args)
if result.return_code:
lock_parse_err = "failed to parse pnpm lock file with yq. '{}' exited with {}: \nSTDOUT:\n{}\nSTDERR:\n{}".format(" ".join(yq_args), result.return_code, result.stdout, result.stderr)
else:
importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None) # NB: yq will return the string "null" if the yaml file is empty
importers, packages, patched_dependencies, lock_parse_err = utils.parse_pnpm_lock_json(result.stdout if result.stdout != "null" else None) # NB: yq will return the string "null" if the yaml file is empty

priv["importers"] = importers
priv["packages"] = packages
Expand Down
3 changes: 0 additions & 3 deletions npm/private/test/BUILD.bazel
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,6 @@ load(":pkg_glob_tests.bzl", "pkg_glob_tests")
load(":transitive_closure_tests.bzl", "transitive_closure_tests")
load(":translate_lock_helpers_tests.bzl", "translate_lock_helpers_tests")
load(":utils_tests.bzl", "utils_tests")
load(":yaml_tests.bzl", "yaml_tests")

# gazelle:exclude **/snapshots/**/*

Expand All @@ -26,8 +25,6 @@ transitive_closure_tests(name = "test_transitive_closure")

translate_lock_helpers_tests(name = "test_translate_lock")

yaml_tests(name = "test_yaml")

parse_pnpm_lock_tests(name = "test_parse_pnpm_lock")

generated_pkg_json_test(name = "test_generated_pkg_json")
Expand Down
61 changes: 0 additions & 61 deletions npm/private/test/parse_pnpm_lock_tests.bzl
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,55 +6,18 @@ load("//npm/private:utils.bzl", "utils")
def _parse_empty_lock_test_impl(ctx):
env = unittest.begin(ctx)

parsed_yaml = utils.parse_pnpm_lock_yaml("")
parsed_json_a = utils.parse_pnpm_lock_json("")
parsed_json_b = utils.parse_pnpm_lock_json("{}")
expected = ({}, {}, {}, None)

asserts.equals(env, expected, parsed_yaml)
asserts.equals(env, expected, parsed_json_a)
asserts.equals(env, expected, parsed_json_b)

return unittest.end(env)

def _parse_merge_conflict_test_impl(ctx):
env = unittest.begin(ctx)

parsed = utils.parse_pnpm_lock_yaml("""
importers:
.:
dependencies:
<<<<< HEAD""")
expected = ({}, {}, {}, "expected lockfileVersion key in lockfile")

asserts.equals(env, expected, parsed)

return unittest.end(env)

def _parse_lockfile_v5_test_impl(ctx):
env = unittest.begin(ctx)

parsed_yaml = utils.parse_pnpm_lock_yaml("""\
lockfileVersion: 5.4

specifiers:
'@aspect-test/a': 5.0.0

dependencies:
'@aspect-test/a': 5.0.0

packages:

/@aspect-test/a/5.0.0:
resolution: {integrity: sha512-t/lwpVXG/jmxTotGEsmjwuihC2Lvz/Iqt63o78SI3O5XallxtFp5j2WM2M6HwkFiii9I42KdlAF8B3plZMz0Fw==}
hasBin: true
dependencies:
'@aspect-test/b': 5.0.0
'@aspect-test/c': 1.0.0
'@aspect-test/d': 2.0.0_@aspect-test+c@1.0.0
dev: false
""")

parsed_json = utils.parse_pnpm_lock_json("""\
{
"lockfileVersion": 5.4,
Expand Down Expand Up @@ -112,34 +75,13 @@ packages:
None,
)

asserts.equals(env, expected, parsed_yaml)
asserts.equals(env, expected, parsed_json)

return unittest.end(env)

def _parse_lockfile_v6_test_impl(ctx):
env = unittest.begin(ctx)

parsed_yaml = utils.parse_pnpm_lock_yaml("""\
lockfileVersion: '6.0'

dependencies:
'@aspect-test/a':
specifier: 5.0.0
version: 5.0.0

packages:

/@aspect-test/a@5.0.0:
resolution: {integrity: sha512-t/lwpVXG/jmxTotGEsmjwuihC2Lvz/Iqt63o78SI3O5XallxtFp5j2WM2M6HwkFiii9I42KdlAF8B3plZMz0Fw==}
hasBin: true
dependencies:
'@aspect-test/b': 5.0.0
'@aspect-test/c': 1.0.0
'@aspect-test/d': 2.0.0(@aspect-test/c@1.0.0)
dev: false
""")

parsed_json = utils.parse_pnpm_lock_json("""\
{
"lockfileVersion": "6.0",
Expand Down Expand Up @@ -196,21 +138,18 @@ packages:
None,
)

asserts.equals(env, expected, parsed_yaml)
asserts.equals(env, expected, parsed_json)

return unittest.end(env)

a_test = unittest.make(_parse_empty_lock_test_impl, attrs = {})
b_test = unittest.make(_parse_lockfile_v5_test_impl, attrs = {})
c_test = unittest.make(_parse_lockfile_v6_test_impl, attrs = {})
d_test = unittest.make(_parse_merge_conflict_test_impl, attrs = {})

TESTS = [
a_test,
b_test,
c_test,
d_test,
]

def parse_pnpm_lock_tests(name):
Expand Down
Loading
Loading