-
Notifications
You must be signed in to change notification settings - Fork 5
Add devshell/package caching #48
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
Conversation
69bf9f0 to
388ad25
Compare
1a56f1e to
4321391
Compare
04653df to
7120300
Compare
|
I will make similar PRs to clash-protocols and for clash-compiler as well if people don't have any objections to this PR. |
|
I wonder how much sense it makes to advertise the local cache like this. Perhaps it should just be a company internal thing that recommends setting up the local cache. What do other people think? |
Yeah I was wondering about that too, but I don't think there's much harm in it and it is a logical place to put the information. If there's a better place to add this information, then I'll happily put it there. UPDATE: I moved the documentation to a small note within the ci.yml file. |
7120300 to
febdb42
Compare
Pushes the Nix build results to the locally hosted binary cache and Cachix. These will pushes will only be done commits on main. Both caches implement a garbage collector which automatically cleans packages which arent being used. Meaning that pushes on main do not waste storage.
Adds a small paragraph about using Cachix to speed up Nix-centric development.
febdb42 to
d6d53cf
Compare
|
Bump Got any opinions on this @diegodiv and @DigitalBrains1 ? If not, then I'd like to merge it |
| extra-substituters = https://clash-lang.cachix.org | ||
| extra-trusted-public-keys = clash-lang.cachix.org-1:/2N1uka38B/heaOAC+Ztd/EWLmF0RLfizWgC5tamCBg= |
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.
Isn't
nix profile install nixpkgs#cachix
cachix use clash-lang
a bit less "intimidating"?
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 get where you're coming from, but at the same time why make people install something when you can use Cachix just fine without having the CLI installed? Alternatively it can be listed as 'you can also do this'.
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.
My preference would be to start out with the solution that is the simplest to perform, so cachix use clan-lang. And then make a note "It is not required to use the cachix package for this; alternatively, you can use the following configuration if you would like to avoid installing it:"
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.
That's a funny divide here because I personally find it simpler not to install the tool at all (unless it's required somewhere else). I never used it, and always added substituters in my config files.
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.
Well I can just remember the instructions to install and use cachix, whereas with the other instructions, I'll really need to look up the instructions or an existing deployment and copy-paste. I'm considering "simple" as "an action that is simple to perform for the user", not "my system is reduced to its simplest state".
But I'm fine with any order; I do think it makes sense to document both so people can pick what most fits their life style.
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 do think it makes sense to document both so people can pick what most fits their life style.
Same!
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've included both methods now
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.
FWIW I generally like "simple" instructions with @DigitalBrains1's definition of simple:
I'm considering "simple" as "an action that is simple to perform for the user", not "my system is reduced to its simplest state".
in READMEs. If it's a race between my desktop installing cachix and my slow ass doing a bunch of edits, I know who's going to win.
Anyway, having both is good. Just figured I'd share my POV.
Reworks the Nix paragraph to include the how to use the Cachix CLI to add the substituters.
.github/workflows/ci.yml
Outdated
| env: | ||
| ATTIC_TOKEN: ${{ secrets.ATTIC_SECRET }} | ||
| run: | | ||
| attic login --set-default public http://192.168.102.136:9200/ "$ATTIC_TOKEN" |
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 way, the secret shows up in the process list if you time it right. While the risk of leakage is small, it is poor form. Does attic also accept a token from stdin? Because you might be able to do
attic login --set-default public http://192.168.102.136:9200/ - <<<"$ATTIC_TOKEN"which, as far as I know, does not leak the token via the process list. My assumption was that a - argument would be interpreted as "take it from stdin", this is pretty standard but not universal.
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.
Ah it also seems possible without relying on bash features:
printenv ATTIC_TOKEN | attic login --set-default public http://192.168.102.136:9200/ -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.
Good catch! I wasn't aware that it could leak like that. Sadly it does not support reading from stdin, but we can write to ~/.config/attic/config.toml directly for the meantime.
I have written a small bash script which writes the config file and the CI invokes the bash script. Would that be leak proof?
I have also made the Cachix authentication use environment variables rather than a direct argument as well.
Related Attic PR: zhaofengli/attic#284
d00faf8 to
e04acb3
Compare
DigitalBrains1
left a comment
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.
Thank you for looking into avoiding the leak of secrets through the process list. I don't think we're dealing with an actual risk here, but it's nicer this way.
.github/workflows/ci.yml
Outdated
| env: | ||
| ATTIC_AUTH_TOKEN: ${{ secrets.ATTIC_SECRET }} | ||
| run: | | ||
| bash .github/scripts/attic-auth.sh |
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.
If you make the script executable and add a shebang, you can just run the script itself rather than call bash. It's more idiomatic that way.
Additionally, we have these scripts in clash-cores/.ci. Maybe once we switch to a full GitHub Actions workflow, we want to move all scripts to a subdir of .github, but currently, I'd much prefer this script to be in ~/.ci as well.
| bash .github/scripts/attic-auth.sh | |
| .ci/scripts/attic-auth.sh |
.github/scripts/attic-auth.sh
Outdated
| endpoint = "http://192.168.102.136:9200" | ||
| token = "$ATTIC_AUTH_TOKEN" | ||
| EOF | ||
|
|
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 file is bracketed by blank lines; please remove leading and trailing blank lines.
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.
Good to know! Personally like trailing blank lines but I will adapt :)
| # Having the variable be named `CACHIX_AUTH_TOKEN` authenticates cachix | ||
| # https://docs.cachix.org/getting-started#authenticating |
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's definitely better to comment too much than comment too little! Having said that, I think this was already clear enough by calling the env var that and subsequently not referencing it directly in the run key :-).
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 it though? If I were to read it without prior context I wouldn't know that the environment variable would get picked up by the cli without being passed explicitly. Could also just be me heh
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.
Could also just be me heh
Or it is that I have gotten used to undercommented CI concoctions 😂
It was just a bit of feedback, not a change request at all. It's fine as it is.
Reworks the authentication method to not leak the secret.
e04acb3 to
12c100c
Compare
Compiles the Nix package and devshells (including HLS) for clash-cores and uploads them to the local binary cache and cachix.
These runners are hosted locally and Diepenheim and only activate on commits to the main branch. I have tested both pushing to the local binary cache and cachix.
This will cache the version of clash-compiler and clash-protocol as well, but only the ones pinned by the flake.lock. Those projects will need their own cache in-order to support different versions too.
TODO