-
-
Notifications
You must be signed in to change notification settings - Fork 108
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
[FR]: link outside WORKSPACE for dependencies during local development (~= pnpm link
)
#1165
Comments
For a non-contrived motivation, included here to avoid cluttering the main description:
|
pnpm link
)pnpm link
)
If you link a package link
However, rules_js doesn't like this and errors out
Seems like this could work hermetically since linking changes the lock file. Would be really nice if we could get support for this feature, since it's a very common workflow for developers who maintain 3rd party packages in their workspaces. I'd be happy to make a PR if someone from aspect could give me a little guidance of how this might be implemented. Please let me know. |
When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running 'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within pkg/ui/workspaces/* creates a symbolic link at node_modules/@cockroachlabs/foo that points to ../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a 'pnpm webpack --watch' running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc. Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked @cockroachlabs/ packages are detected. [1] https://pnpm.io/cli/link [2] aspect-build/rules_js#1165 Release note: None Epic: none
When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running 'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within pkg/ui/workspaces/* creates a symbolic link at node_modules/@cockroachlabs/foo that points to ../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a 'pnpm webpack --watch' running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc. Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked @cockroachlabs/ packages are detected. [1] https://pnpm.io/cli/link [2] aspect-build/rules_js#1165 Release note: None Epic: none
107110: kvserver: avoid running intensive decommission test under deadlock r=AlexTalks a=AlexTalks The kvserver test `TestDecommission`, which runs a 5-node cluster and decommissions 4 of those 5 nodes, has trouble completing fast enough when under a race or deadlock configuration. While race configurations were already skipped, this modifies the test to be skipped under deadlock configurations as well. Fixes: #106096 Release note: None 107111: compose: start `docker-compose` with a non-empty `PATH` r=rail a=rickystewart `docker-compose` invokes `docker`, but obviously this will fail if there is nothing in the `PATH`. Epic: none Release note: None 107129: dev: reject builds when CRL JS dependencies are 'pnpm link'ed r=rickystewart a=sjbarag When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running `pnpm link /path/to/github.com/cockroachdb/ui/packages/foo` from within pkg/ui/workspaces/* creates a symbolic link at node_modules/`@cockroachlabs/foo` that points to ../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a `pnpm webpack --watch` running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc. Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow `pnpm link` to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked `@cockroachlabs/` packages are detected. [1] https://pnpm.io/cli/link [2] aspect-build/rules_js#1165 Release note: None Epic: none ----- Example output: <img width="998" alt="image" src="https://github.com/cockroachdb/cockroach/assets/665775/3fd43abe-f5c2-4ddd-bc60-16a73db12836"> Total duration is 16ms on my machine since we're checking so few files. We can drop these into a goroutine per first-party JS if we want, but this is certainly fast enough to be conversational. 107149: opt: make functional dependency calculation deterministic r=DrewKimball a=DrewKimball We recently added additional logic for inferring functional dependencies for join expressions. However, this logic iterates through a map, which leads to nondeterminism in which order functional dependencies are added to the FD set. Functional dependency calculation is best-effort, so this can lead to a different resulting FD set, which causes flaky tests. This patch makes the calculation deterministic by iterating instead through a `intsets.Fast` set. Fixes #107148 Fixes #107162 Release note: None 107181: dev: when cross-building, use `-c opt` r=rail a=rickystewart This enables optimizations which you probably want for a cross-build. Epic: CRDB-17171 Release note: None 107183: acceptance: add log dir as a writable path r=rail a=rickystewart Otherwise you get a sandbox error. Epic: CRDB-17171 Release note: None Co-authored-by: Alex Sarkesian <sarkesian@cockroachlabs.com> Co-authored-by: Ricky Stewart <ricky@cockroachlabs.com> Co-authored-by: Sean Barag <barag@cockroachlabs.com> Co-authored-by: Drew Kimball <drewk@cockroachlabs.com>
When working with first-party JS dependencies that aren't in this monorepo, the idiomatic development workflow uses pnpm link [1] to link multiple JS packages together. Specifically, running 'pnpm link /path/to/github.com/cockroachdb/ui/packages/foo' from within pkg/ui/workspaces/* creates a symbolic link at node_modules/@cockroachlabs/foo that points to ../../(…)/ui/packages/foo. This works quite smoothly for local development, as changes in the 'foo' package are automatically seen by a 'pnpm webpack --watch' running in CRDB. The two packages act like they're maintained in the same repo, while allowing independent version history, CI processes, etc. Unfortunately, rules_js currently offers no way to link outside of the Bazel workspace. Such a symlink is either ignored by rules_js (since it doesn't appear in pnpm-lock.yaml) or is rejected if it's manually added to the lockfile [2]. Allowing Bazel-based builds of CockroachDB when 'pnpm link'ed packages are present introduces dependency skew between Bazel and non-Bazel builds. To allow 'pnpm link' to be used safely, pre-emptively reject builds of 'cockroach' and 'cockroach-oss' that are run through the 'dev' helper when linked @cockroachlabs/ packages are detected. [1] https://pnpm.io/cli/link [2] aspect-build/rules_js#1165 Release note: None Epic: none
I think it makes sense to allow you to do this. We've also discussed a bazel wrapper (like Aspect CLI) could give an easier way to apply patches to third-party libraries - imagine you have edits in anything that bazel downloader fetches and could easily go from the repo where you made the edits to a patch file Bazel applies. (issue 828 in our internal monorepo) |
@sjbarag you should be able to use the I'm not sure if that actually resolves the request, or if the typical |
Thanks, that gets pretty close! It does risk someone committing that change, but it'd of course fail in CI. Perhaps that's not as big a deal as I'd originally thought. The main benefits for
Of course, those same symlinks can become a nightmare with Node's resolution algorithm. One can easily end up with multiple copies of |
Maybe I'm misunderstanding, but I still get the "Invalid link_package outside of the WORKSPACE" error when I try this. For context, I add the absolute path to the root of the external module in the pnpm overrides:
And then when I do
|
I think maybe aspect-build/bazel-examples#290 illustrates a solution for this? @gregmagolan is it the same issue? |
One big constraint here is that Bazel can't reference targets outside of the WORKSPACE so you can't just put another repo on disk and pull it into the Bazel graph. That is why an absolute path such as You can bring other repositories on disk into a Bazel graph with Long story short, it is tricky to reproduce the |
There may be a pattern where you In this use case, is the dep being |
…col for package.json versions that point to packages external to the workspace
In our case we are developing an open source library without bazel, but want to be able to test integrating it into our internal bazel powered monorepo without publishing a release of the library to npm, and so being able to have the file linkages would be really nice. |
What is the current behavior?
Currently,
rules_js
fetches packages from either:npm_translate_lock
.Attempts to link outside the Bazel workspace to an NPM package located elsewhere on-disk get rejected with
invalid link_package outside of the WORKSPACE.
Describe the feature
The npm link workflow is pretty common in the node community, as it allows a dev to modify a third-party package and use it without having to vendor that package.
As a contrived example motivation (replace
left-pad
with any other package):left-pad
performance that my application highlightsleft-pad
. While working on that fix, I want to use the locally-changed copy in my application as a sort of "real world" gut-check in addition to theleft-pad
test suite. Just to make sure my pathological case is indeed getting fixed.left-pad
for any number of reasons:left-pad
has a complicated/obscure build system that makes it non-trivial to vendor and build withrules_js
.left-pad
takes bugfixes incredibly fast, so my vendored copy would only exist for a few hours after my PR goes out.In the current state, there appears to be no way to build
left-pad
in a separate directory and use it in myrules_js
-built package(s).With vanilla
pnpm link
(ornpm link
or …), the workflow is roughly:pnpm --dir ~/bitbucket.org/my-org/my-app link ~/github.com/left-pad/left-pad
to use a customleft-pad
left-pad
.my-app
. It includes myleft-pad
changes.Ideally, the workflow would be pretty similar under Bazel + rules_js. For local development it feels reasonable to break hermeticity, akin to
npm_translate_lock
's currentuse_home_npmrc
), but I'd be happy to add an extra flag or two to achieve this.Relevant slack conversation: https://bazelbuild.slack.com/archives/CEZUUKQ6P/p1689358470077099
The text was updated successfully, but these errors were encountered: