mattwparas / steel

An embedded scheme interpreter in Rust
Apache License 2.0
1.05k stars 49 forks source link

fix `flake.nix` - install `cargo-steel-lib` properly during `steel` build #183

Closed cgahr closed 3 months ago

cgahr commented 5 months ago

This closes #182.

I create a new package cargo-steel-lib with is then used as a native build input for the actual installation of steel.

Minor changes:

cgahr commented 5 months ago

@mattwparas I have some questions regarding cargo-steel-lib.

When do we need cargo-steel-lib? Only during installation? Or also during development? If it is the former, we don't need an individual cargo-steel-lib package and simplify the steel package. If we need cargo-steel-lib during development, however, it makes sense to keep those two separate.

mattwparas commented 5 months ago

@mattwparas I have some questions regarding cargo-steel-lib.

When do we need cargo-steel-lib? Only during installation? Or also during development? If it is the former, we don't need an individual cargo-steel-lib package and simplify the steel package. If we need cargo-steel-lib during development, however, it makes sense to keep those two separate.

So - as it is implemented now, cargo-steel-lib is used for building shared dylibs. So if you're interested in building dylibs you'll probably want to have that installed. Also, as of now, it is needed to install some of the baseline packages steel uses. I would like these to be optional and more configurable, but for now thats how it is set up.

Its in my short term todo list to move that into the steel interpreter itself; so that way you can do something like steel dylib build rather than needing to invoke cargo steel-lib

cgahr commented 5 months ago

In this case, I think the best option is to build cargo-steel-lib together with steel itself and use it to install the steel base packages.

After the installation is done, we can delete the cargo-steel-lib executable, but we can also keep it. Here, I'm unsure what the best path of action is (probably we should just keep it, it doesn't really hurt to have it after all). Either way, it's just about whether we keep line 44 in flake.nix.

In case everything looks good, please don't merge just yet, I'd like to think a bit more about whether everything makes sense or not. It is, however, ready for review.

cgahr commented 5 months ago

@mattwparas I think this is ready to merge, pending your review.

mattwparas commented 4 months ago

This looks good to me - do you mind if we merge the one that this conflicts with first?

cgahr commented 4 months ago

Sounds good to me, any merge conflict should be easy to solve!

cgahr commented 3 months ago

rebased to current master and fixed all conflicts, ready to review @mattwparas

mattwparas commented 3 months ago

rebased to current master and fixed all conflicts, ready to review @mattwparas

Thanks! What is the best way to test this? Eventually I'd like a CI step as well to test that this builds correctly

nxy7 commented 3 months ago

I'm not an PR author but you could test building and running the derivation in one step with nix shell github:cgahr/steel#steel -c steel --help (in CI obviously the github link would be replaced with relative path .#steel). I think just checking the status code of this command would give good indication whether it's building correctly.

By the way I think that it would be nice to make the flake runnable via nix run github:cgahr/steel#steel. If I'm not mistaken the only thing that needs to be done to do that would be changing the line here to equal to steel or maybe manifest.bin.name.

mattwparas commented 3 months ago

I'm not an PR author but you could test building and running the derivation in one step with nix shell github:cgahr/steel#steel -c steel --help (in CI obviously the github link would be replaced with relative path .#steel). I think just checking the status code of this command would give good indication whether it's building correctly.

By the way I think that it would be nice to make the flake runnable via nix run github:cgahr/steel#steel. If I'm not mistaken the only thing that needs to be done to do that would be changing the line here to equal to steel or maybe manifest.bin.name.

Thanks - I was able to test with this - @cgahr do you mind making the change to make the flake runnable via nix run with the change above?

cgahr commented 3 months ago

I'm not an PR author but you could test building and running the derivation in one step with nix shell github:cgahr/steel#steel -c steel --help (in CI obviously the github link would be replaced with relative path .#steel). I think just checking the status code of this command would give good indication whether it's building correctly.

I think this would work. There is one big issue, however: this requires GitHub to rebuild steel from scratch, which takes a while and is resource intensive. I don't know whether it should run as a job.

By the way I think that it would be nice to make the flake runnable via nix run github:cgahr/steel#steel. If I'm not mistaken the only thing that needs to be done to do that would be changing the line here to equal to steel or maybe manifest.bin.name.

You're right, this would be nice to have. However, I think something like

...
apps.steel = {
  type = "app";
  program = "${steel}/bin/steel";
};
apps.default = apps.steel;

would be the proper way to do this. Alas, the following commands work on the most recent commit:

nix run github:cgahr/steel
nix run github:cgahr/steel#steel

@nxy7 Thanks for your pointer!

mattwparas commented 3 months ago

I'm not an PR author but you could test building and running the derivation in one step with nix shell github:cgahr/steel#steel -c steel --help (in CI obviously the github link would be replaced with relative path .#steel). I think just checking the status code of this command would give good indication whether it's building correctly.

I think this would work. There is one big issue, however: this requires GitHub to rebuild steel from scratch, which takes a while and is resource intensive. I don't know whether it should run as a job.

By the way I think that it would be nice to make the flake runnable via nix run github:cgahr/steel#steel. If I'm not mistaken the only thing that needs to be done to do that would be changing the line here to equal to steel or maybe manifest.bin.name.

You're right, this would be nice to have. However, I think something like

...
apps.steel = {
  type = "app";
  program = "${steel}/bin/steel";
};
apps.default = apps.steel;

would be the proper way to do this. Alas, the following commands work on the most recent commit:

nix run github:cgahr/steel
nix run github:cgahr/steel#steel

@nxy7 Thanks for your pointer!

Let's get this merged then while I think on adding it as a CI step; it would just double the CI time as is. Might be worth running separately as like a nightly job if there have been any commits in the last day to have a gut check on any regressions with the build for nix

Otherwise could have it run if you change the nix file (something like that)

nxy7 commented 3 months ago

Small comment from me - since nix can build derivations in isolation you could set up cache and build things locally (assuming you are running Linux) and they would be shared with CI. If I remember correctly I've done something like that once since usually PCs are much faster than github CI runners. It would have the added benefit that users of the flake maybe could get away without building it.

I think you've mentioned that you're not using NixOS so maybe that wouldn't apply to you specifically @mattwparas but still setting up Cachix could make some of CI runs quick.

It's not something that you'd have to do right now (or ever) just it's worth keeping in mind as an option if you'll think that Nix CI steps are taking too long.

EDIT. running on nix file changes would not be sufficient since the underlying code can be changed (like steel dependencies) without changes in nix files