Repository navigation
Conversation
|
Codex usage limits have been reached for code reviews. Please check with the admins of this repo to increase the limits by adding credits. |
|
Please fix the failing snapshot tests which will also provide a diff of the real results of this change helping the review process |
| ":.aspect_rules_js/node_modules/@rollup+rollup-win32-ia32-msvc@4.55.2": "@rollup/rollup-win32-ia32-msvc", | ||
| }, | ||
| "@aspect_rules_js//platforms/pnpm:win32_x64": { | ||
| ":.aspect_rules_js/node_modules/@rollup+rollup-win32-x64-gnu@4.55.2": "@rollup/rollup-win32-x64-gnu", |
There was a problem hiding this comment.
Do we need the same fix for windows? 😨
There was a problem hiding this comment.
Not with the data we have. @rollup/rollup-win32-x64-gnu and -msvc both declare only os: [win32] and cpu: [x64] in the lockfile, with no libc or other field to tell them apart, so pnpm installs both on win32 x64 as well. Choosing one would need a mapping from package names to toolchains that pnpm doesn't have, so I've left windows as is.
|
|
||
| for k, v in deps.items(): | ||
| if k in deps_oss and k in deps_cpus: | ||
| if k in deps_oss and k in deps_cpus and k in deps_libcs: |
There was a problem hiding this comment.
What if we have k in deps_cpus and k in deps_libcs but not k in deps_oss? Is that impossible and libc constraints only exist if os+cpu constraints also exist?
Either way it makes me wonder if this 3-if/else-block is no longer the ideal way to structure this code. I'm not sure if you have a better idea or if it's necessary at the time but please give it a thought...
There was a problem hiding this comment.
Possible, yes: npm doesn't require os or cpu alongside libc, though every native package I've seen declares all three. Before this fix, such a dependency fell back to the os/cpu-only branch and the libc was dropped.
I replaced the if/elif chain with pnpm.to_bazel_constraints(oss, cpus, libcs), which returns the select() group and the conditions for one dependency. _to_deps_attr now just buckets dependencies by group. Any dependency that declares libc goes in the libc group. A missing os or cpu means all of them, as it does in pnpm, so cpu: [x64], libc: [musl] becomes darwin_x64, win32_x64, … plus linux_x64_{musl,unconstrained}. aix maps to the linux constraint, so it's folded into linux to keep the conditions within one select() from overlapping. Covered in constraints_test and os_cpu_libc_constraints_test. The rollup snapshot is unchanged by the refactor.
There was a problem hiding this comment.
The generated Next.js links snapshot also needs to be updated. The current CI failure shows examples/nextjs/snapshots/next_links_defs.bzl still expecting the old linux_<cpu> selects while the generated output now contains linux_<cpu>_{glibc,musl,unconstrained}:
https://github.com/aspect-build/rules_js/actions/runs/36410446225/job/109039252901
The failure suggests running:
bazel run //nextjs:write_next_links_defs--codex
| resolution = package_data["resolution"], | ||
| cpu = package_data.get("cpu", None), | ||
| os = package_data.get("os", None), | ||
| libc = package_data.get("libc", None), |
There was a problem hiding this comment.
libc needs normalization here because it is valid as either a string or a list. npm's documented example uses "libc": "glibc", and pnpm preserves that scalar form; this repo already has examples in e2e/pnpm_lockfiles/v101/pnpm-lock.yaml (and v110/v120). As written, a scalar flows into deps_libcs, which is later passed to attr.string_list_dict() (or would be iterated character-by-character by _resolve_pnpm_constraint_values), so valid projects such as ones using sass-embedded will fail repository evaluation. Could we normalize a scalar to [value] and add coverage for both forms?
--codex
There was a problem hiding this comment.
Good catch, thanks. os, cpu and libc are now normalized to a list when the lockfile is read (_constraint_list in pnpm.bzl), so the scalar form used by sass-embedded in e2e/pnpm_lockfiles/v120 works. I did os and cpu too because package.json allows a string for them as well. The new parse test covers both the scalar and the list form.
fe10b72 to
28fa927
Compare
npm_translate_lock built each optional dependency's select() from its os and cpu alone, so on linux a package with glibc and musl builds linked both. It now reads libc from the pnpm-lock.yaml as well and, on linux, keys those dependencies on new linux_<cpu>_<libc> config settings over //platforms/libc. A platform that does not set //platforms/libc matches the unconstrained setting and links every variant, as before. Fixes aspect-build#3040
- pnpm keeps a package's [os], [cpu] and [libc] as written, and package.json allows a string as well as a list (sass-embedded uses "libc": "glibc"). Normalize all three to a list when reading pnpm-lock.yaml. - Replace the if/elif chain in _to_deps_attr with pnpm.to_bazel_constraints, which returns a dependency's select() group and conditions. A dependency that declares [libc] with only one of [os] and [cpu] now goes in the libc group too: a missing [os] or [cpu] allows all of them, as in pnpm, and aix folds into linux so the conditions in one select() never overlap. - Regenerate examples/nextjs/snapshots/next_links_defs.bzl.
28fa927 to
0b6e263
Compare
|
Rebased on main. I regenerated |
0b6e263 to
3c121b7
Compare
Fixes #3040.
npm_translate_lockbuilt each optional dependency'sselect()from itsosandcpualone, so on linux a package that ships glibc and musl builds linked both (for example@rollup/rollup-linux-x64-gnuand-musl, or@anthropic-ai/claude-agent-sdk-linux-x64and-linux-x64-musl, about 235 MB each).Changes
pnpm.bzlreadslibcfrompnpm-lock.yamlpackages, besideosandcpu._collect_dep_constraintscollects it, and it reachesnpm_import_links_ruleas a new internaldeps_libcsattr, the same waydeps_ossanddeps_cpusdo._to_deps_attrputs dependencies that declareos,cpuandlibcin their ownselect(). On linux they are keyed on new@aspect_rules_js//platforms/pnpm:linux_<cpu>_<libc>config settings over the existing//platforms/libcconstraint. Other OSes keep<os>_<cpu>, since pnpm only checkslibcon linux.Each such dependency is also keyed on
linux_<cpu>_unconstrained. A platform that doesn't set//platforms/libc(its default isunconstrained) still links every variant, so nothing changes unless a platform asks for glibc or musl.docs/pnpm.mdgets a short "Platform-specific packages" section showing how to set it.pnpm.to_bazel_constraintspicks each dependency'sselect()group and conditions, which replaces the if/elif chain in_to_deps_attr. A dependency that declareslibcwithoutosorcpuis grouped with the libc dependencies too: a missingosorcpuallows all of them, as in pnpm, andaixfolds into linux so that conditions never overlap.os,cpuandlibcare normalized to lists when the lockfile is read, since package.json allows a string as well ("libc": "glibc"insass-embedded).Test plan
os_cpu_libc_constraints_testinpnpm_test.bzl;parse_pnpm_lock_testsexpects the newlibcfield.constraints_test, more cases inos_cpu_libc_constraints_test, and a parse test for scalar and listos/cpu/libc.examples/nextjs/snapshots/next_links_defs.bzlregenerated;//nextjs:write_next_links_defs_testpasses on bazel 7.7.1.snapshots/rollup_links_defs.bzlregenerated: thelinux_*entries move into the libcselect().cqueryon//:.aspect_rules_js/node_modules/rollup@4.55.2's direct deps, with a throwaway platform on the host:rollup-linux-x64-gnuandrollup-linux-x64-musl, as before//platforms/libc:glibc:rollup-linux-x64-gnuonly//platforms/libc:musl:rollup-linux-x64-muslonlybazel test //npm/... //platforms/...: the 125 tests that load pass. A few targets under the root package didn't load where I ran this, because it couldn't fetch the example packages that come from GitHub commit tarballs; none of them involve this change.