feat(nix): overhaul nix flake and packages #9032
Conversation
the whole point of having a lock file is to have a stable set of packages to develop against. bun was recently updated to 1.3.6 in nixpkgs-unstable so until opencode moves to 1.3.6 we should stay pinned to a commit of nixpkgs that has bun 1.3.5 if automated flake lock updates are desired, the update should at least be conditioned on the apps still building
|
Thanks for your contribution! This PR doesn't have a linked issue. All PRs must reference an existing issue. Please:
See CONTRIBUTING.md for details. |
|
The following comment was made by an LLM, it may be inaccurate: Related PR Found:
The current PR #9032 is not a duplicate but rather a successor to #8998. It appears #8998 should likely be closed in favor of this more comprehensive nix overhaul. |
|
Yes would love to hear feedback from the other nix folks |
|
I was hoping someone else who better knew what they were doing would take over the nix code, thank you 😄 Works great on my machine |
gigamonster256
force-pushed
the
push-osnrnvnwzxsz
branch
2 times, most recently
from
aea0dd0 to
db2812b
Compare
|
Now is probably a good time to update the meta description too. From the current README: |
gigamonster256
force-pushed
the
push-osnrnvnwzxsz
branch
10 times, most recently
from
5ff415e to
9cd7b85
Compare
It's a step in the good direction at removing a lot of unneeded existing build indirections, I will review it thoroughly ASAP. |
Please, authorize the CI workflow to run to avoid blind review. |
There was a problem hiding this comment.
The namespace collision is a showstopper for binary resolution in the PATH on macOS for example when both packages are installed.
The rest is mainly cleanup + robustness review comments.
Since you seem to have the knowledge and the time, it would be great if you can implement standard checks (formatting, build, derivation, ...) the nix way: the CI and devs would only need to do nix check to run a comprehensive validation of the nix opencode packaging :)
nix/desktop.nix
Outdated
| mainProgram = "opencode-desktop"; | ||
| platforms = platforms.linux ++ platforms.darwin; | ||
| license = lib.licenses.mit; | ||
| mainProgram = "OpenCode"; |
There was a problem hiding this comment.
Namespace collision, introduce breakage on case insensitive FS and existing installation, etc. as seen in #8998.
If it's a recent change in the desktop app on the binary name output, it should be fixed (and would never have been accepted). If it's an already existing longtime issue, it should be reported and fixed ... (and it's a bit worrying, binary namespace for cross platform is well documented in the literature).
Please workaround it for now.
There was a problem hiding this comment.
sure, I can make a workaround for now.
it looks to be a change as of 96975ef - before it had a space in it which is perhaps it was falling back to the rust package name of opencode-desktop. This should definitely be raised as a new issue to see if co-located cli and desktop installs are supported
to note, the desktop packages distribute the opencode binary as "opencode-cli"
The whole project dependencies handling should be handled that way :) A tool like renovate or dependabot should be properly configured and open PRs for deps updates that MUST pass the CI before being merged (including nix packages) |
gbpdt
left a comment
gbpdt
left a comment
There was a problem hiding this comment.
Looks like a great cleanup!
|
Thank yall lmk how I can help |
gigamonster256
force-pushed
the
push-osnrnvnwzxsz
branch
3 times, most recently
from
1d3cd21 to
0342bb3
Compare
gigamonster256
force-pushed
the
push-osnrnvnwzxsz
branch
2 times, most recently
from
4e72506 to
632fd0a
Compare
standardizes dev packages to the regular callPackage style and brings best practices from nixpkgs upstream maintains compatibility with current nix based CI updates to node_modules hashes
gigamonster256
force-pushed
the
push-osnrnvnwzxsz
branch
from
632fd0a to
7305d3a
Compare
5 participants
standardizes dev packages to the regular callPackage style and brings best practices from nixpkgs upstream
maintains compatibility with current nix based CI updates to node_modules hashes
NOTE: this removes the flake-update CI job - read the commit desc for why - open to discussion
Superscedes #8998
would love feedback/get testing from recent nix committers @gbpdt @Alb-O @jerome-benoit