Implement workspace build separation#82
Implement workspace build separation#82poly2it wants to merge 7 commits intonix-community:masterfrom
Conversation
baileylu121
left a comment
There was a problem hiding this comment.
This is great work, since it's a rather large PR I'm planning to have this be version 2.1.0 once it's ready, bear with me if it takes a couple iterations, but me and I'm sure many others appreciate it a lot.
|
|
||
| # Copy workspace root lockfile into the build directory if provided. | ||
| if [ -n "${bunLockFile-}" ]; then | ||
| echo "Copying workspace lockfile into build directory..." |
There was a problem hiding this comment.
This (and the latter echo) should probably be a proper setup hook log:
You can see the ones provided by nixpkgs for any setup hook here
| fi | ||
|
|
||
| echo "Constructing node_modules from cache (no network)..." | ||
| @modulePopulator@ --cache-dir "$BUN_INSTALL_CACHE_DIR" |
There was a problem hiding this comment.
Generally, I do prefer the approach of building node_modules ourself, in fact, the original v1 did it this way. The issue with it, however is there are a bunch of hard to find edge cases that crop up from time to time.
I think the best compromise between the two is to catch any exit codes from the module populator and fall back to the current implementation since the union of the two is likely to be able to handle pretty much anything.
Perhaps even a manual configuration flag in mkDerivation would be ideal to allow users to chose between the two.
There was a problem hiding this comment.
Maybe we should use module-populator by default, but provide an option like useLegacyCachePopulator or useBunForCachePopulation, which could be automatically suggested in the error message in module-populator fails.
bun2nix attempted to populate the module cache without calling to Bun but failed. Consider filing an issue on [...] and use [option] to enable the fallback populator.
| src = ../../programs; | ||
|
|
||
| cargoLock = { | ||
| lockFile = ../../programs/Cargo.lock; |
There was a problem hiding this comment.
Instead of manually specifying the src path twice this should probably just be ${finalAttrs.src}/Cargo.lock.
Same for workspace-promoter
| ]; | ||
| } | ||
| // lib.optionalAttrs (bunLockFile != null) { | ||
| bunLockFile = "${bunLockFile}"; |
There was a problem hiding this comment.
If the goal of stringifying this was to force the path to get copied into the store, you can use pkgs.copyPathToStore instead
| Consider updating your local package or contributing to `bun2nix` if this version hasn't been supported yet" | ||
| )] | ||
| UnsupportedLockfileVersion(u8), | ||
| #[error(transparent)] |
There was a problem hiding this comment.
Seeing as the majority of the custom errors we need have been moved to the library, I'd be totally down with dropping this and using something like color-eyre for the command line utils, but it's something I can follow on with myself if you don't want to include it as part of this PR.
| } | ||
|
|
||
| /// Parse the metadata object from a package tuple element. | ||
| fn parse_meta(value: &Value) -> Option<PackageMeta> { |
There was a problem hiding this comment.
This can largely be done with serde's derive macros by just specifying a custom deserialize for the BinField type.
|
|
||
| let lock_contents = fs::read_to_string(&cli.lock_file)?; | ||
|
|
||
| println!("Reading lockfile: {}", cli.lock_file.display()); |
There was a problem hiding this comment.
These entries should probably be produced by log, since we already depend on it anyway
|
|
||
| for pkg in &packages { | ||
| match &pkg.kind { | ||
| PackageKind::Workspace { path } => { |
There was a problem hiding this comment.
This function feels rather long, this is more of a nitpick/style thing but I'd rather see it broken up into smaller pieces
| /// Validate that a bin command name is safe. | ||
| /// | ||
| /// Rejects names containing `/`, `\`, null bytes, `.`, `..`, or empty strings. | ||
| fn validate_bin_name(name: &str) { |
There was a problem hiding this comment.
As with above this should probably say in the function name it does a bunch of assertions or just return a Result
| /// | ||
| /// Uses `cp -rL` to dereference symlinks and `--no-preserve=mode,ownership` | ||
| /// to ensure files are writable (Nix store files are read-only). | ||
| pub fn copy_package(cache_path: &Path, target_path: &Path) { |
There was a problem hiding this comment.
If we're going to do a full copy anyway they should probably be reflinks and/or hardlinks just in case the filesystem supports that.
You can see how bun does it here.
We needed to use: nix-community/bun2nix#82 To workaround: nix-community/bun2nix#77 A bit hacky but it works.
We needed to use: nix-community/bun2nix#82 To workaround: nix-community/bun2nix#77 A bit hacky but it works.
Right now, building a workspace member means pulling in the entire workspace root. Every member shares one derivation, so touching
packages/librebuildspackages/apptoo. This PR makes it possible to build each member on its own, with its own derivation and minimal closure.The new
mkDerivationparameters for the extended workspace support are all optional. Existing derivations that don't use them should be unaffected. That said, we recognise you the maintainers may want to reshape the API.This PR also creates a Rust tool,
module-populator, which prepopulatesnode_moduleswithout runningbun. It copies packages straight out of the pre-fetched Nix cache instead.bun installstill runs afterward, but only to write Bun's internal metadata. This change was largely motivated by trying to solve an issue we were hitting internally that looks very similar to #71,bun installtrying to fetch deps over the network even though the cache already has them. By populatingnode_modulesourselves beforebun installruns, bun no longer needs to resolve anything from the network. This PR resolves those issues for us, but we haven't tested it against the exact reproduction in #71 yet.A per-member build does roughly this:
bun.lockinto the member's build dir and strips sibling workspace deps frompackage.json.module-populatorwalksbun.lock, finds each package in the Bun cache using the@@@marker entries, and copies it intonode_modules/.workspace-promoterrewritesbun.lockto make the target member look like the root package. It collects dependencies from stripped siblings and merges them into the promoted root so transitive resolution still works.bun install --ignore-scriptsruns to reconcile Bun's internal state.node_modulessymlink is placed at the source root so bundlers can resolve imports from sibling code.This made bun2nix work better for our internal usage in a monorepo at @skeptiva. The
workspace-memberexample roughly emulates our usage.