-
-
Notifications
You must be signed in to change notification settings - Fork 16.3k
pkgs/build-support/node: add fetchBunDeps and bun build hooks #414837
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
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool stuff ! Thanks !
Very nice first PR :)
A couple of hints:
- Make sure this PR only contains 1 single commit
- Pass your
.sh
files throughshfmt
andshellcheck
- Add an entry in the release note
a1381b7
to
38153d8
Compare
# Use a constant HOME directory | ||
local home_dir | ||
home_dir=$(mktemp -d) | ||
export HOME="$home_dir" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Perhaps you could add the writableTmpDirAsHomeHook
and get rid of this?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this the right convention? (Still learning.) With or without it, I’ve been running into the following issue:
> Using Bun temp directory: /build/.home/.bun-tmp
> bun install v1.2.14 (6a363a38)
> error: bun is unable to write files to tempdir: AccessDenied
Yarn doesn’t use writableTmpDirAsHomeHook
, it just sets HOME
to a temporary directory directly using export HOME=$(mktemp -d)
. NPM does something similar by setting export HOME="$TMPDIR"
in its config hook.
In my case, I’m using writableTmpDirAsHomeHook
, which sets HOME
and then creates a .bun-tmp
directory inside it. The hook itself is working: it's creating a directory at $NIX_BUILD_TOP/.home
, which resolves to /build/.home
. Bun is trying to create its temp directory inside that, at /build/.home/.bun-tmp
.
But right now, bun still throws a permission error when trying to write to that path. Essentially. I need to do some more research on this on the bun and nix side. I'm going to try the forceing of it to humor my self, but i'd like to follow standards.
d148e47
to
5738263
Compare
Add support for building Bun projects in Nix, following the same pattern as fetchYarnDeps. Includes: - fetchBunDeps: fetches Bun dependencies for offline builds - prefetch-bun-deps: CLI tool for generating dependency hashes - fixup-bun-lock: rewrites bun.lock URLs to local paths - bunConfigHook: sets up offline dependency cache - bunBuildHook: runs bun build command - bunInstallHook: installs built packages Based on fetchYarnDeps implementation but adapted for Bun's JSONC lockfile format and dependency structure.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi, very solid looking implementation, I just have a couple comments about things I learned working on a similar implementation outside of nixpkgs.
--frozen-lockfile \ | ||
--no-progress \ | ||
--no-save \ | ||
--offline \ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bun does not have a --offline
flag, but also does not error if one is given so that it can be aliases as node properly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey, thanks for the detailed feedback, i built this to learn a bit, but now i'm fairly invested, so i appreciate it The --offline
was a sanity check for me, i'm in debug phase right now (i'm pretty sure i'm getting killed by the node_moduels stuff). I saw the last comment here, oven-sh/bun#7956, but have not had time to go through and try. Will go through rest of your feedback, to see if there is a fun way to make it work I guess.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What I ended up doing for mine was a manual implementation of bun install
in nix which might be useful to reference here, however I purposely didn't contribute to nixpkgs with it since a. it looks like it would probably be a big maintenance burden and someone is likely to do a proper --offline
flag for bun eventually and b. it wasn't feature complete i.e missing support for stuff like git packages
fi | ||
|
||
if ! type node > /dev/null 2>&1; then | ||
echo "bunConfigHook WARNING: a node interpreter was not added to the build, and is probably required to run 'bun $bunBuildScript'. A common symptom of this is getting 'command not found' errors for Nodejs related tools." |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Bun has a --bun
flag which can be used to provide an alias to bun for node in these cases. Consider mentioning it in this error message
# SC2154: bunBuildFlags is referenced but not assigned | ||
# This variable is provided by the Nix build environment, not assigned in this script | ||
if [[ -n "${bunBuildFlags-}" ]]; then | ||
bun run "$bunBuildScript" "${bunBuildFlags[@]}" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bun run "$bunBuildScript" "${bunBuildFlags[@]}" | |
bun "$bunBuildScript" "${bunBuildFlags[@]}" |
Bun actually doesn't need the run prefix and will look for scripts in your package json if it is not a built in command
i.e.
bun run xyz
is equal to
bun xyz
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No harm in using run, though right? pnpm behaves the same way, and I tend to include it out of habit. Just wondering, is omitting run the preferred style in Bun?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've seen both but I believe omitting it is preferred and run is included so bun can be aliases to node although I may be wrong
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I believe it's preferable to use bun run
, in case a package.json defines a script sharing a name with another subcommand (eg. bun build
vs. bun run build
)
if [[ -n "${bunBuildFlags-}" ]]; then | ||
bun run "$bunBuildScript" "${bunBuildFlags[@]}" | ||
else | ||
bun run "$bunBuildScript" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
bun run "$bunBuildScript" | |
bun "$bunBuildScript" |
|
||
# Create a bunfig.toml file to explicitly configure Bun | ||
cat > "$PWD/bunfig.toml" << EOF | ||
[install] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As far as I am aware, unfortunately bun currently can't install from an offline cache and properly guarantee that nothing is fetched without a workaround, you can opt to install it manually and create node_modules
yourself or wait for offline tarball support.
See here
@@ -0,0 +1,124 @@ | |||
#!/usr/bin/env node |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Less of a suggestion, more of a question, why not use bun here for this instead of node, considering this is the bun package builder?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yeah, this was the original plan, then i started facing issues, i wanted to get something working first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This seems to use substantial amount stuff from my pr #376299.
Please add co author Co-authored-by: eveeifyeve <[email protected]>
or restore the original commit please so I equally get credited for my work as well as well avoid a potential license violation.
Hey @Eveeifyeve, which part specifically are you referring to? I hadn’t seen your MR until it was linked (probably should've searched), so I didn’t knowingly reuse any commits from it. Since this wasn’t based on your branch, there’s no “original commit” from me to revert—just my own work so far. That said, if there’s meaningful overlap (I'll take a look shortly), I’m happy to credit you as a co-author to keep things fair l. If your commit aligns with what I’m doing, especially around fetchBunDeps, I can cherry-pick it directly to include your work. Otherwise, if the approaches are similar, co-authorship still might makes sense. That said, this might not land as cleanly as #376299, but the fetchBunDeps approach seems to be working, so I think there's still a solid path forward. |
I don't know how useful it can be to this PR, but a while back I tried to make my own implementation of a nix dependency fetcher for bun, but way more similar (in terms of api) to pnpm's I did some minor testing at the time to try to make sure the FOD would build the same on different architectures and have since been using my fetchdeps in a private project which has not failed me so far, despite multiple rebuilds on different nixpkgs revisions. What I made can be found on my git forge: https://git.uku3lig.net/ISITECH/tcl-guessr/src/commit/15a46e05c05e8a01706262fb909b86203017e536/nix/derivation.nix#L14-L50 (Please note that the other private package I have has a more "traditional" installPhase which copies node_modules into the store and uses makeWrapper for the entrypoint, which has worked flawlessly in my usecase) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hey @Eveeifyeve, which part specifically are you referring to? I hadn’t seen your MR until it was linked (probably should've searched), so I didn’t knowingly reuse any commits from it.
oh nvm I just saw configHook and though that was the same code, my bad I approve of this, but I would say a suggestion move this the by-name of bun package to make developers find this easier.
Hey, I noticed an issue when using this PR to build one of my Bun apps. With this When I removed the (also thanks for this PR, i've been stuck for months trying to properly package my Bun apps) |
Also, after fixing this the installation fails with another error:
If it helps, the error persists after adding I've exported the full nix log at https://pastebin.com/Ny5Pk0d5, the project is https://github.com/versia-pub/server/tree/refactor/packages but I haven't committed the port to this PR yet so I've pasted my modified |
What's needed to push this forward? |
Add support for building Bun projects in Nix, following the same pattern as fetchYarnDeps. Includes:
Based on fetchYarnDeps implementation but adapted for Bun's JSONC lockfile format and dependency structure.
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.