-
Notifications
You must be signed in to change notification settings - Fork 86
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
Add flake for supporting NixOs and nix systems #110
Conversation
I am not sure what is the testing process if any, but let me know. Also I had no chance to try out the new bash path in any other systems than ubuntu and nix, so let me know if there is any problem with it in any other systems. |
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.
It seems that the new bash path works. Interesting to see NixOS support! I've no way to test this but I trust you that it works. I'll add the README part to documentation once this gets merged
Great, what are the next steps? (I also highly recommend trying out NixOs and nixpgs in general, pretty cool to do development on especially for command line) |
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.
im not sure that its necessary to pollute repo's root with files. can you create a directory structure like this?
(root)
|- packages
|--- nix
|----- default.nix
|----- ...
also is it really necessary to host this kind of thing on a main repo? don't packages usually have their own?
Typically the |
@uncenter It is possible with appending @b1ek The nixpkgs can wrap your release with nix for sure, but it is not preferred (you need to have a maintainer updating versions and dependencies for this repo in a third party repo). This way it is also easy and unified to set up development env for Amber. (If you do not include it everybody on nix will have to write it himself, gitignore it before able to work on the project). That's said these files are in root because they are just like |
@brumik does this installer build the Amber from scratch or does it use the latest release on GitHub? I'm trying to figure out how to solve it. By no means I've never used NixOS but by searching things on the web I've seen some people do some custom Bash in the |
It is not bash, but sh. Bash is not the default. You can of course run commands when building the package but in this case it is not needed. That's more of a case when you wrap an existing project and you need to do some more advanced manipulations with the compile targets. It is hard to wrap your head around Nix but Nix is Source based and not Package based 'distro'. This means that the user compiles the program on their machine from a public repo URL. So in the example in readme it is using the main branch. But you can also specify a release branch or release tag or just a plain commit SHA. As this project is not Nix fist project I would expect having Tags for each release that people on nix could reference. (There are of course compiled binaries on a cache server for nixpkgs since you don't want to compile chrome and such on your machine, but that's catching detail and not how nix works in the first place.) |
The problem is, if everything gets thrown into repo's root it will make a horrible mess out of it. By "everything", i mean that Nix would not be the first distro to host package configs in the root of the repo. I mean, just a few files is not a problem, but unspeakable horrors will unfold when 10 more distros or packaging systems like Nix will put their files in the root. What about creating a separate repo only with config files and having github actions fetch the source & config files for each release/commit? That way, its easier to file issues and/or PRs about the package, the main repo is clean and nice and Nix packaging system is happy |
There is no GitHub action that needs to be run. Flake.nix is telling NixOS what is needed to compile the code and how to compile it. So putting in new repo makes sense if the source code is in the new repo (aka a fork with one extra commit that needs to be synced) |
We can probably remove A example repo with super similar setup (CMD line tool written in rust) is https://github.com/jrmoulton/tmux-sessionizer |
@brumik will this package require future maintenance or we can just leave this flake file and call it a day? Does the installer recompile from source and update versions and tags on each install? |
For the most part, no. Some rare cases where updating might be needed.
This flake instructs Nix how to build it from source, so yeah "recompile from source" is true but I'm not sure what you mean "update versions and tags on each install". |
@uncenter I meant that it updates the Amber version automatically in the nix pkg registry when it detects that the |
Currently there are two versions of the Amber package - one is upstream in https://github.com/NixOS/nixpkgs, which is auto-updated by a bot that detects when you release a new version (I am a maintainer for this package FYI), and the other is this new flake in this repository. This flake will always be fully up to date. |
naersk.url = "github:nix-community/naersk/master"; | ||
nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable"; | ||
utils.url = "github:numtide/flake-utils"; |
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 don't see much point in adding flake-utils or naersk. But it's fine, we can certainly add them.
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.
Perhaps we could add an overlay above the nixpkgs package amber-lang
Hi, sorry to absolutely drop a massive review, but I worked with the team packaging this for upstream nixpkgs, and I would prefer it was done correctly here to. Thanks for doing this though. |
@isabelroses It is much appreciated, great place to learn more. |
They did this with neovim before they moved the flake off the main repo |
Thanks @isabelroses for your feedback! |
I updated the PR and removed the default.nix and shell.nix files. @isabelroses and @b1ek could you give it one more look? |
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.
Builds and runs correctly
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.
lgtm, but the readme is kind of a mess
README.md
Outdated
|
||
The package is already containing all the required install scripts and dependencies. You can use the flake as: | ||
|
||
```nix |
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 all that info really necessary? if thats just a package, you should mention it with one line under the "Via package manager" header:
Nix - amber-lang
if you really need this info, you could wrap it in a dropdown box so it doesnt take up much space
like this
<details>
<summary>Title</summary>
### Put stuff here
</details>
im just saying that not every amber user is a nix user. they don't need this information.
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.
also nothing is preventing you from creating a NIX.md
file near README.md
with the necessary instructions
README.md
Outdated
@@ -37,6 +38,26 @@ Amber is packaged in the following distros: | |||
|
|||
Arch (AUR) - `amber-bash-bin` | |||
|
|||
#### Nix | |||
|
|||
The package is already containing all the required install scripts and dependencies. You can use the flake as: |
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.
grammar: package is not "is already containing", it "contains"
Yeah @brumik please move the installation details to |
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.
LGTM
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.
lgtm
This PR adds Nix and NixOs support for amber. It packages the executable with
bc
andbash
.In addition it fixes the
/bin/bash
executable path since it is not supported on all systems (for example nix) replacing it with the truly universal/usr/bin/env bash
.The readme is updated to reflect NixOs possible install process (this is highly individual as nix is doing source based package management and all you need is the structure of the flake.nix).
shell.nix
anddefault.nix
are there for thenix-shell
andnix-build
(in case not using flakes)..envrc
is allowing the use of direnv for easy development setup on nix.Closes #109