WIP: bun: init configHook and fetchDeps; bun: switch to finalAttrs from rec#376299
WIP: bun: init configHook and fetchDeps; bun: switch to finalAttrs from rec#376299Eveeifyeve wants to merge 4 commits intoNixOS:masterfrom
Conversation
This comment was marked as spam.
This comment was marked as spam.
This comment was marked as outdated.
This comment was marked as outdated.
5b658cf to
fac0e2a
Compare
winterqt
left a comment
There was a problem hiding this comment.
To even begin reviewing this, this needs to be properly split up into multiple commits; ideally, the Bun upgrade would be done in a separate PR entirely.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as outdated.
a72a721 to
30e4175
Compare
This comment was marked as outdated.
This comment was marked as outdated.
Merged |
|
Just re-requesting for reviews that are outdated from the following reviewers or have been made a topic already about and have been marked resolve because it's a duplicate.
|
|
You do realise the code isn't evaluating right? |
This comment was marked as resolved.
This comment was marked as resolved.
| bunWorkspaces = [ "./packages/opencode" ]; | ||
| bunDeps = bun.fetchDeps { | ||
| inherit (finalAttrs) | ||
| pname | ||
| version | ||
| src | ||
| bunWorkspaces | ||
| ; | ||
| hash = "sha256-HGoeaGrrWYIuaI9yEQJNadCUmmsgFYiQW56ginUa1Bk="; |
There was a problem hiding this comment.
This change will probably break opencode-desktop since it inherits node_modules from opencode and depends on more than just ./packages/opencode. However, I'm currently not a fan of how opencode and opencode-desktop are two separate packages but depend so heavily on each other (both need to be updated at the same time).
There was a problem hiding this comment.
The only thing that is breaking is opencode dependency out of this.
I will probably look into a separate hook for the bun deps for desktop still dependent on src and version from opencode. However probably best in a separate issue to separate the dependent of opencode version src and stuff and make it separate.
There was a problem hiding this comment.
Only thing is left to do is fix opencode, so I do the hash for desktop.
There was a problem hiding this comment.
Right, I just tested the build and got
specified: sha256-HGoeaGrrWYIuaI9yEQJNadCUmmsgFYiQW56ginUa1Bk=
got: sha256-bxDOFMH70AivlhQpxbyDE+hI4372GveEGxjkEQGOA9w=
There was a problem hiding this comment.
Even when it unpacks through that phase still broken as the tarball is not unpacking before it gets installed.
delafthi
left a comment
There was a problem hiding this comment.
opencode LGTM. Just added a comment about opencode-desktops dependency to opencode.
|
pyrox0
left a comment
There was a problem hiding this comment.
Found some grammar fixes and one small change I'd like to be made, or at least considered.
|
|
||
| #### Dealing with CPU/OS specific dependencies {#javascript-bun-oscpu} | ||
|
|
||
| Some bun dependencies may require OS or CPU architecture specific dependency variations. With the `os` and `cpu` attributes in `fetchDeps`, you can specify which to fetch. By default the fetcher will get all CPU architectures for linux, darwin and freebsd. |
There was a problem hiding this comment.
All other fetchers fetch all dependencies regardless of platform. Is this an option for bun? Making this non-configurable leads to fewer code paths and thus less room for error and bugs to creep in. Also, fetching only specified platforms means if we change some default "blessed" set of platforms, we'd have to regenerate all hashes that depend on that set, whereas fetching all platforms means that hashes won't change in those cases(because they would not exist due to there being no blessed set of platforms we fetch for by default)
There was a problem hiding this comment.
The thing is that there is no such default. Bun will by default only pull dependencies for the current system. That makes the output non-deterministic. Also pulling in windows dependencies wouldn't make much sense as these take up a lot of space for nothing. But we also have nixpkgs pkgs that only support one arch or platform. It wouldn't make much sense to also pull in unused code.
These are in essence fetcher parameters and it is expected that the output will change when the input parameters change.
Other fetchers generally only fetch source code and not binaries and are plotform/os agnostic.
|
This should really use |
|
The FODs produced are not reproducible. The build fails if you do not have the FOD as it reports a different hash. |
RossSmyth
left a comment
There was a problem hiding this comment.
Think very carefully about the public API. What should be able to be overriden? What shouldn't be? Making the public API as small as possible for the MVP is good because expanding it later is not a breaking change, while reducing it is.
Also giving this a try, the FODs are not reproducible as noted above.
| dontConfigure = args'.dontConfigure or true; | ||
| dontFixup = args'.dontFixup or true; | ||
|
|
||
| inherit outputHash outputHashAlgo; |
There was a problem hiding this comment.
Please actually think about this review instead of saying that is is complicated. It is making it more future-proof.
For this I have the check with other apps that use bun and see if it's globally producing There is also some known issues I am trying to fix before this like in the configHook it's not unpacking the tar correctly. |
The dd command is just writing 4 bytes at offset 96 in the file. Which does fit very nicely with my diff where I found the difference at 0x60. This would be valid as long as we assume that the position of the timestamp is always at that static offset and that we are dealing with a 4 byte little endian encoded unix timestamp. Considering that there were 4 trailing 0 bytes, we might be dealing with an 8 byte value. We definitely need to look into the binary format. Also I don't expect the second command restoring the date to really work since it seems to try to read the current date via the date command. |
| # `bunDeps` should be a tarball | ||
| unpackFile "$bunDeps" | ||
| bunDepsCopy="$(realpath "$(stripHash "$bunDeps")")" | ||
|
|
||
| echo "Configuring bun store" | ||
|
|
||
| HOME=$(mktemp -d) | ||
| BUN_INSTALL_CACHE_DIR=$(mktemp -d) | ||
| export HOME BUN_INSTALL_CACHE_DIR | ||
|
|
||
| cp -Tr "$bunDepsCopy" "$BUN_INSTALL_CACHE_DIR" | ||
| chmod -R +w "$BUN_INSTALL_CACHE_DIR" |
There was a problem hiding this comment.
Why exactly are we creating 2 copies of the unpacked FOD? Couldn't we just unpack into a temp dir and point BUN_INSTALL_CACHE_DIR at it?
There was a problem hiding this comment.
It needs to first unpack the deps to $bunDeps and then next line stripes the hash before it can be copied to the BUN_INSTALL_CACHE_DIR.
There was a problem hiding this comment.
Hold on, aren't we unpacking from bunDeps to the current working directory? The location of the folder is then stored in bunDepsCopy where the content is then copied to Bun_install_cache_Dir.
This seems a bit unnecessarily complicated.
Why not something like this:
HOME=$(mktemp -d)
pushd $(mktemp - d)
unpackFile "$bunDeps"
export BUN_INSTALL_CACHE_DIR="$(realpath "$(stripHash "$bunDeps")")"
popd
There was a problem hiding this comment.
It supposed to be unpacking to $bunDeps. The reason as to why it's not is because it doesn't have a root dir. Please have a look at @getchoo review if your unsure.
There was a problem hiding this comment.
| fi | ||
|
|
||
| # `bunDeps` should be a tarball | ||
| unpackFile "$bunDeps" |
There was a problem hiding this comment.
Known problem: This is not unpacking to $bunDeps which is not correct in fetcher.
Co-authored-by: Winter <winter@winter.cafe> Co-authored-by: Seth Flynn <getchoo@tuta.io> Co-authored-by: kuflierl <41301536+kuflierl@users.noreply.github.com>
Adds
bun.fetchDeps&bun.configHookto bun and updates bun to 1.2.Blocked by Mic92/nix-update#557 for nix-update-script's.
This pr will lead up to the building bun from source.
NOTE: If you have something to say in this pr please think about if it's spam, off-topic, has already been said (which may have a comment about why I have resolved it) or would be preferred to be a message in matrix, before commenting as this pr. As this pr is becoming big with a lot of people subscribed and I am trying to keep it organized, maintained and not ping heavy.
All related issues that it closes:
closes #335534
closes #255890
closes #414837
closes #424038
This work is semi sponsored by DigitalBrewStudios.
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-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.