-
Notifications
You must be signed in to change notification settings - Fork 76
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
Simplified nix flake for build, dev shell, and cached build #74
base: main
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.
This is a really awesome pr! I've used nix a little bit but not enough to know the proper ways to do things. Ss there some kind of way to make sure this builds correctly in our CI?
flake.nix
Outdated
buildPhase = '' | ||
export CARGO_HOME="/tmp/.cargo" | ||
export RUSTUP_HOME="/tmp/.rustup" | ||
cargo build --features all |
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 going to try to build it with llama.cpp support? I don't think we want that by default. The default features are what we should have built by default.
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 will be tweaking somethings and posting an update. This was a hacky way of going about it. I am updating another PR to a nixpkgs submission that is more idiomatic and will try to update this flake accordingly :)
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.
Sounds good
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.
removed a bunch of unnecessary components and did some refactoring
5e04161
to
7c44328
Compare
7c44328
to
db9f1cf
Compare
in | ||
rec { | ||
packages = rec { | ||
default = pkgs.rustPlatform.buildRustPackage { |
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 is the main
package. Essentially when running nix build
locally, the default
entry is the starting point. The devPackage and devShells are more of conveniences for being dropped into a shell with all needed dependencies, but still track a cached build at /tmp/.cargo
so during the dev cycle, one can run cargo
commands as they please with the necessary caches.
nativeBuildInputs = commonNativeBuildInputs; | ||
buildInputs = commonBuildInputs; | ||
|
||
doCheck = false; |
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.
doCheck = false;
Disables all testing. See nixpkgs implementation to see how to disable individual tests. As this nix flake is most likely not going to be integrated into any CI, I think it is okay to disable them for now since the goal is just for folks who want to pull and have a dev env/build from source avenue. Suggestions welcome.
I am not sure I understand: Is there a desire to use nix instead for CI? As these changes aren't necessary or intended to replace a working CI, unless it's needed. |
Hey! Love to see work on packaging Here are some differences between our flakes that I think would be beneficial to add:
I'm not the most knowledgeable, but I'm happy to try and answer any questions or help research things we both know! |
Currently working through the nixpkg approval process. It is involved to say the least. I think there might be a difference between the two goals. I mainly just created this flake as a semi easy way to setup a dev environment. I definitely think it can be improved based on the feedback from the nixpkg review process. I think a solution lies somewhere in-between what you have and what I posted |
I think he's referring to ensuring that the nix package builds (which theoretically could fail even if the rust package can compile via the normal imperative commands). There seems to be some documentation already over using nix in CI, |
That is exactly what I am referring to. I'm a bit worried to merge this in without actually using nix myself and having no way to verify that it is consistently working. I know a lot of people do use it and I would love to have a flake, but I want to make sure it is sustainable. |
Since I've set up my flake to be mainly for package distribution and this PR set this up to be for the devShell, I could make make a PR to this PR (or directly contribute, whatever is preferred) with parts of my lsp-ai flake and also a GitHub Actions CI for building the nix package if @ProjectInitiative is open to working together! |
Chiming here to show interest. Any updates on this? @ProjectInitiative @mergemoveagree |
If we get something tested and working for a few people I'm happy to merge it in! I'm not very experienced with writing my own flakes though so heavily relying on everyones thoughts here. |
Update on my side - my bad, actually - that |
It isn't obsolete necessary, there might be a desire from a dev perspective to have a nix-ified dev shell. From a consumer environment, yes, the nixpkg version (slightly different to conform to nixpkg standards) is gtg for consumers of the package. I haven't had a ton of time to test this specifically since I haven't needed to contribute to the project codebase directly. |
I see #60. Honestly not sure which method is better at the moment. This PR allows local building with
nix build
and a development shell withnix develop
. Ideally adding an entry to nixpkgs might solve #60 and #32 since it will be built into the eco-system. Open to discussion. Still learning nix myself.