-
Notifications
You must be signed in to change notification settings - Fork 384
support a nix-shell environment for editors #2302
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: main
Are you sure you want to change the base?
Conversation
@MagnusS I fixed the issues we talked about. It was fairly trivial once I figured it out. I had manually appended |
811d5e0
to
a1a84e9
Compare
a1a84e9
to
227c66c
Compare
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 tested this now and this looks like a very nice development workflow! When I ran it, it created ./build where I could rebuild IncludeOS itself. I was also able to build the example unikernel separately in the same shell (in ./example/build) by just doing cmake -B build && make && boot ...
.
I think we can merge this as is - but It would be nice with a bit more documentation in the README with details on the expected workflow - how do you iterate over changes in the kernel, how do you build/rebuild a unikernel that depends on it. If you can run editors with LSP inside the shell that's also useful to include :-)
Should we enable ccache by default for the development shell? It requires additional setup the first time, which is not super friendly for newcomers; but I would argue it's almost necessary for quick iteration which we're promising. |
3bb1daf
to
6fd120f
Compare
6fd120f
to
9d3ac26
Compare
I'm fine with changing the default if it's well documented and the failure mode is easy to understand. For example the error message if ccache isn't found should probably mention that you can disable it with a parameter: https://github.com/includeos/IncludeOS/blob/main/overlay.nix#L72. In case you want clean builds, it could also be useful with a reminder on entering the shell that ccache is enabled (and an explanation for how to disable it). |
3ed42f4
to
6da243c
Compare
6da243c
to
c1d4e42
Compare
I can't find a nice way to print a notice that the ccache is used, mainly due to the flooding of logs (maybe we should forward logs to a file instead?). If one inspects the logs, one will find the notice quite early in the logs (once per includeos build—i.e. once for i686 and once for x86_64), but we're not printing them at all if we never reach a ccache invocation at all. Maybe this is fine? I've added a message on how to disable the ccache if it's not configured on the system, so that's nice. |
Since
shell.nix
is already occupied for building bootable unikernel images, this makes quick iteration development on the unikernel itself a bit awkward.To work around this, I've created
develop.nix
(in spirit of the experimentalnix develop
command from nix3/flakes) which puts the editor in a non-pure environment shell. As it stands, this builds IncludeOS by itself without a bootable image, generating a nix-fixedcompile_commands.json
which is consumable by clangd and by extent most editors, and includes some handy packages on the shell.Usage is fairly trivial:
nix-shell ./develop.nix
. It makes little sense to use this in--pure
mode, as we assume you have an editor installed on your own system.Notes
We should consider extending this environment with
.lldbinit
(or.gdbinit
, but I would argue the llvm debugger makes more sense for us) or the like. Maybe @torgeiru has some tips on what we should include there.I'm not entirely happy with the repetition of
packages
in bothshell.nix
anddevelop.nix
, but I guess it makes sense as we're sort of misusingshell.nix
for the purpose of what should be another repository, ideally. One can always callnix-shell
ornix-shell --run 'boot ./hello_includeos.elf.bin'
from within thenix-shell ./develop.nix
shell.Source declarations for IncludeOS are local, while source declarations for patched musl (i.e. libc/libcxx) is in the nix store. It doesn't make too much sense to point towards the non-patched musl nor our patches; so we might as well just point this to the nix store even if this makes quick iteration a bit annoying.