Skip to content
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.lock & workflow to update it #1934

Merged
merged 2 commits into from
Aug 22, 2022
Merged

Conversation

i077
Copy link
Contributor

@i077 i077 commented Apr 16, 2021

Description

This PR implements suggestions from #1913, adding a flake.lock file so we can nix run github:nix-community/home-manager without any hiccups. The nixpkgs input is currently set to track nixos-unstable. If you want to test this out, you can run nix run github:nix-community/home-manager/pull/1934/head, which should give you the help text.

To keep this file up to date, the PR also adds a GitHub workflow that

  • Runs every Sunday and Wednesday at 03:51 (I just picked a random time)
  • Runs nix flake update --commit-lock-file, committing as a nonexistent user called flakebot
  • Pushes this commit directly to master to a new branch called flake-update with a custom token FLAKEBOT_TOKEN (this needs to be added to the repo)
  • Opens a PR (or updates an existing PR) to merge this commit to master. This is done with the same custom token so that other workflows can be run against the PR.

Also updates the README to change the section on a flake-based config a bit.

I also wanted to modify the testing workflow to actually run home-manager's tests, but was having a bit of trouble exposing the tests under the checks output of the flake, so I will leave that out for now. This means that the update workflow will just regularly push updates to master, whether they pass tests or not.

cc @berbiche since we were talking about this in the other PR.

Checklist

  • Change is backwards compatible.

  • Code formatted with ./format.

  • Code tested through nix-shell --pure tests -A run.all.

  • Test cases updated/added. See example.

  • Commit messages are formatted like

    {component}: {description}
    
    {long description}
    

    See CONTRIBUTING for more information and recent commit messages for examples.

  • If this PR adds a new module

    • Added myself as module maintainer. See example.

    • Added myself and the module files to .github/CODEOWNERS.

@i077 i077 requested a review from rycee as a code owner April 16, 2021 16:36
@teto
Copy link
Collaborator

teto commented Apr 18, 2021

at some point it was discussed to open a PR instead of an automatic merge. I preferred that scenario, is that doable through github actions ? nevermind the checks, they can be added later

@i077
Copy link
Contributor Author

i077 commented Apr 19, 2021

I can have the workflow checkout -B a new branch and open a new PR, that's what @kclejeune and I do on our nixos configs. My only concern was that I didn't want the repo to just have a bunch of flake update PRs, but if you're okay with that I can implement it.

edit: I should clarify: no more than 1 flake update PR will be open at a time, but it will require manual merging each time it gets opened (at most twice a week).

@teto
Copy link
Collaborator

teto commented Apr 19, 2021

edit: I should clarify: no more than 1 flake update PR will be open at a time, but it will require manual merging each time it gets opened (at most twice a week).

That seems the best option, would it close and open a new PR or force-push to the current one ?

@i077
Copy link
Contributor Author

i077 commented Apr 19, 2021

If the update action runs and there's already an existing PR, it will just force-push that branch. The -B switch in the checkout command basically "force creates" a new branch, even if it already existed in the remote, so the previous update commit just gets overwritten.

Also remember that a new secret containing a github token with push access to the repo needs to be made so we can trigger other workflows on the PR. I'd like to add the checks attribute to the flake with all of home-manager's tests so we can actually run them against the nixpkgs input in the update PR, but like I said I was having trouble with that so it can come later.

I'll update the workflow to push to a new branch later today.

@i077 i077 force-pushed the flake-lock branch 3 times, most recently from 1c5b7e8 to 29daba6 Compare April 20, 2021 20:26
README.md Outdated
home-manager.url = "github:nix-community/home-manager";
home-manager = {
url = "github:nix-community/home-manager";
inputs.nixpkgs.follows = "nixpkgs";
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is potentially harmful, advanced users can use it but we should default to the mainstream method.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

could you elaborate on how this is potentially harmful?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

home-manager is guaranteed to work with the nixpkgs in its lock file, it has not been tested with the overriden version so there is a risk of failure no ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

IMO users typically expect their entire configuration to be evaluated against a single instance of nixpkgs unless they specify otherwise, so I figured overriding the input would be a sensible thing to include here.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I disagree, that's not how flakes are conceived (it doesn't default to a single nixpkgs) and I think users prefer something that work rather than shaving off a few MB on the disk. It's still possbile but shouldn't be the default I think.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kclejeune, maybe I'm confused. I don't see any way to create a token for an organization. What I can see is a way to create a secret, but then I have to provide a value. Perhaps I don't have sufficient access to the nix-community organization? In the documentation I found https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization but it seems to be limited to GITHUB_TOKEN.

Copy link

@kclejeune kclejeune May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@kclejeune, maybe I'm confused. I don't see any way to create a token for an organization. What I can see is a way to create a secret, but then I have to provide a value. Perhaps I don't have sufficient access to the nix-community organization? In the documentation I found https://docs.github.com/en/organizations/managing-organization-settings/disabling-or-limiting-github-actions-for-your-organization but it seems to be limited to GITHUB_TOKEN.

edit: it's not possible the dummy account is probably the way to go until I can finish the flakebot app in a week or two

Copy link
Contributor Author

@i077 i077 May 2, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think this is possible, personal access tokens are per-user only, not per-organization. I am an owner of another GitHub org and I see no such option to create a token at the org level.

The org can have secrets that are maintained at the org level, but their values (in our case: access tokens) would be generated per-user.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we fix this ? that's the last issue before merging I think ? @Mic92 you seem to have the necessary rights, haven't you ?

.github/workflows/update.yml Outdated Show resolved Hide resolved
.github/workflows/update.yml Outdated Show resolved Hide resolved
PULL_REQUEST_FROM_BRANCH: flake-update
PULL_REQUEST_BRANCH: master
PULL_REQUEST_UPDATE: true
PASS_ON_ERROR: true
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why do we need this?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will open a new pull request against the master branch, or, if one exists already, update the existing one. PASS_ON_ERROR is set to true since this action will always be run, regardless of whether the flake was updated during the workflow. So if there were no changes, this step would fail, but we don't want the workflow to fail because of that.

Copy link
Contributor

@asymmetric asymmetric May 11, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we instead check if there was any update, and optionally skip this step? Because I'm not sure if we should have a blanked ignore of all errors. Could be hard to notice when there are actual issues.

One way to do this is checking the commit_info declared above.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hmm, this will be a lot harder than just checking commit_info, since that is literally the commit info of HEAD, regardless of whether a flake update was committed or not. I suppose I can check if a commit was made by comparing against github.sha, though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking about it some more, I think a better way to do this would be to ask the author of the action to let us specify to pass just if there are no new commits, that way I don't have to "reinvent the wheel". I can open an issue for that.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Action author has suggested that I just implement this logic in this workflow's step, so I have pushed 0be2c37. @asymmetric can you check if this looks right?

.github/workflows/update.yml Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
flake.nix Outdated
@@ -29,10 +31,9 @@
hm = import ./modules/lib { lib = nixpkgs.lib; };
homeManagerConfiguration = { configuration, system, homeDirectory
, username, extraModules ? [ ], extraSpecialArgs ? { }
, pkgs ? builtins.getAttr system nixpkgs.outputs.legacyPackages
, check ? true, stateVersion ? "20.09" }@args:
, pkgs ? nixpkgsFor.${system}, check ? true, stateVersion ? "20.09"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a nit, but why do we need to use a custom function when nixpkgs.legacyPackages.${system} would suffice?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Figured it was a more succinct expression that was being used elsewhere anyways. I can revert this particular change, though.

@asymmetric
Copy link
Contributor

This PR implements suggestions from #1913, adding a flake.lock file so we can nix run github:nix-community/home-manager without any hiccups.

If this is the only reason, wouldn't it make more sense to fix it on nix's side? Why does nix run try to save a lock-file in the first place? And shouldn't this be configurable somewhere?

@i077
Copy link
Contributor Author

i077 commented May 7, 2021

This PR implements suggestions from #1913, adding a flake.lock file so we can nix run github:nix-community/home-manager without any hiccups.

If this is the only reason, wouldn't it make more sense to fix it on nix's side? Why does nix run try to save a lock-file in the first place? And shouldn't this be configurable somewhere?

See the discussion in NixOS/nix#4699.

git checkout -B flake-update
- name: Update & commit lockfile
run: |
nix flake update --commit-lock-file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nix flake update will update all inputs. At the moment it's only nixpkgs, but if we added more, that could be surprising.

Suggested change
nix flake update --commit-lock-file
nix flake lock --update-input nixpkgs --commit-lock-file

Copy link
Contributor Author

@i077 i077 May 12, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The PR body will list the updated inputs (actually, it just inserts the auto-generated commit message), so it won't be as surprising. I think it's better that we update all inputs if more get added so we don't have a situation where inputs haven't been updated in weeks or months.

@teto
Copy link
Collaborator

teto commented Jun 14, 2021

what is blocking ? looking forward to this to make lockstep updates in home-manager. Right now, when the nixos-unstable channel updates it can very well break home-manager. It would be better if we could choose what version of nixos-unstable works well via the flake.

@Mic92
Copy link
Member

Mic92 commented Jun 16, 2021

what is blocking ? looking forward to this to make lockstep updates in home-manager. Right now, when the nixos-unstable channel updates it can very well break home-manager. It would be better if we could choose what version of nixos-unstable works well via the flake.

From my understanding it requires a github token still? I can add one on every level.

@Mic92
Copy link
Member

Mic92 commented Jun 26, 2021

If you need any organisational changes please drop a message in #nix-community:bethselamin.de or
#tmp-nix-community:numtide.com on matrix

@i077
Copy link
Contributor Author

i077 commented Sep 8, 2021

Has a token been added as a secret to this repo? What else needs to be done to move this PR forward?

@Mic92
Copy link
Member

Mic92 commented Nov 4, 2021

Has a token been added as a secret to this repo? What else needs to be done to move this PR forward?

No. I have received no token so far. I think the best solution would be if one of the home-manager maintainer create a bot account that we add to the nix-community organisation.

@stale
Copy link

stale bot commented Feb 2, 2022

Thank you for your contribution! I marked this pull request as stale due to inactivity. If this remains inactive for another 7 days, I will close this PR. Please read the relevant sections below before commenting.

If you are the original author of the PR

  • GitHub sometimes doesn't notify people who commented / reviewed a PR previously when you (force) push commits. If you have addressed the reviews you can officially ask for a review from those who commented to you or anyone else.
  • If it is unfinished but you plan to finish it, please mark it as a draft.
  • If you don't expect to work on it any time soon, please consider closing it with a short comment encouraging someone else to pick up your work.
  • To get things rolling again, rebase the PR against the target branch and address valid comments.

If you are not the original author of the PR

  • If you want to pick up the work on this PR, please create a new PR and indicate that it supercedes and closes this PR.

@Atry
Copy link

Atry commented May 2, 2022

When I tried to execute:

nix run github:nix-community/home-manager --no-write-lock-file

I got:

warning: not writing modified lock file of flake 'github:nix-community/home-manager':
• Added input 'nixpkgs':
    'github:NixOS/nixpkgs/b283b64580d1872333a99af2b4cef91bb84580cf' (2022-05-01)
error: attribute 'defaultApp.x86_64-linux' should have type 'derivation'

However, if I execute:

nix shell github:nix-community/home-manager --no-write-lock-file -c home-manager

It works well.

I think we might also want to add a defaultApp to flake.nix

@terlar
Copy link
Contributor

terlar commented May 2, 2022

This is already provided (defaultApp). What you are seeing is a recent change in Nix 2.8 where the defaultApp needs to be a derivation as opposed to an attribute set that was the convention before.

@i077
Copy link
Contributor Author

i077 commented May 3, 2022

@DeterminateSystems have since released a GitHub Action that does what we would want, and it's probably better for home-manager to use that rather than maintain its own workflow. I can update this workflow to using that action instead.

It will use a default automated token for opening update PRs, but we'll still need to provide a personal auth token if we want to run tests against these PRs.

@ncfavier ncfavier mentioned this pull request May 20, 2022
5 tasks
@ncfavier
Copy link
Member

#2971 adds a flake.lock, so I guess it's blocked on this.

@DeterminateSystems have since released a GitHub Action that does what we would want, and it's probably better for home-manager to use that rather than maintain its own workflow. I can update this workflow to using that action instead.

Yes, I think that's better.

It will use a default automated token for opening update PRs, but we'll still need to provide a personal auth token if we want to run tests against these PRs.

This is currently not needed as the CI tests don't use the flake at all (maybe they should...).

This is already provided (defaultApp). What you are seeing is a recent change in Nix 2.8 where the defaultApp needs to be a derivation as opposed to an attribute set that was the convention before.

Fixed in #2971, see #2442 (comment).


About the interval, I think twice a week is excessive considering that the updates require human intervention. Once every one or two weeks seems about right, maybe less often for release branches if we choose to update them (maybe release branches should follow the appropriate nixos- release branch instead of unstable?).

@i077
Copy link
Contributor Author

i077 commented May 23, 2022

This is currently not needed as the CI tests don't use the flake at all (maybe they should...).

My hope is that we'd later port rycee's home-manager testing framework to support flakes in a later PR. I was working on that very briefly, but stopped a while ago because of schoolwork.

About the interval, I think twice a week is excessive considering that the updates require human intervention. Once every one or two weeks seems about right, maybe less often for release branches if we choose to update them (maybe release branches should follow the appropriate nixos- release branch instead of unstable?).

I agree that release branches should follow the corresponding branch in nixpkgs. In particular, I think

  • master should follow nixpkgs-unstable since home-manager is developed against that branch anyway, and
  • release-xx.yy should follow nixos-xx.yy.

I suppose that means, going forward from release-21.11, we should change the nixpkgs input in flake.nix to follow the appropriate branch and cherry-pick backports on top of that as usual. (Cutting new release branches would involve updating flake.nix at that point as well.)

However, that would mean maintainers would have to review 2 PRs per run of this workflow (for master and current release branch).

@Mic92
Copy link
Member

Mic92 commented May 29, 2022

I added a secret for a github bot in this repo. If you use the GH_TOKEN_FOR_UPDATES secret than you will get a token of this bot https://github.com/home-manager-bot
This bot can create branches on this repo

@Mic92
Copy link
Member

Mic92 commented May 29, 2022

@ncfavier
Copy link
Member

ncfavier commented Jun 7, 2022

#2971 was merged, we have a flake.lock now.

@i077
Copy link
Contributor Author

i077 commented Jun 9, 2022

Thank you @ncfavier and @Mic92 for your help. A lot has happened since I opened this PR, so later today I will rebase my branch onto the tip of the master branch and redo most of the work so that it only adds the update workflow, since we now have a flake.lock.

As I mentioned somewhere earlier in this PR, future work will involve porting home-manager's tests to the checks output of flake.nix, so we can run CI tests against flake updates.

@i077
Copy link
Contributor Author

i077 commented Jun 10, 2022

Alright, branch rebased. Since most of the work in the rest of the old commits was done elsewhere, this PR now just implements that update workflow. PRs will be tagged with the "dependencies" label (like dependabot does) to distinguish it from other PRs.

.github/workflows/update-flake.yml Show resolved Hide resolved
.github/workflows/update-flake.yml Outdated Show resolved Hide resolved
@ShamrockLee
Copy link
Contributor

ShamrockLee commented Jun 16, 2022

The tests would be available through the legacyPackages output after #3024 #3082 is merged.
It can be run automatically by

$ nix develop .#tests.run.all

README.md Outdated Show resolved Hide resolved
@teto
Copy link
Collaborator

teto commented Aug 22, 2022

tempted to merge this (and if it breaks stuff we'll deal with it). Any objection ?

@i077
Copy link
Contributor Author

i077 commented Aug 22, 2022

I'm fine with that. I will just draw attention to #1934 (comment), in particular whether release branches should receive a similar update workflow, perhaps in a later PR:

I suppose that means, going forward from release-21.11, we should change the nixpkgs input in flake.nix to follow the appropriate branch and cherry-pick backports on top of that as usual. (Cutting new release branches would involve updating flake.nix at that point as well.)

However, that would mean maintainers would have to review 2 PRs per run of this workflow (for master and current release branch).

We don't have to address this now, but perhaps if we merge this PR we should consider doing the above on the next NixOS release.

@teto teto merged commit 0160a0c into nix-community:master Aug 22, 2022
GaetanLepage pushed a commit to GaetanLepage/home-manager that referenced this pull request Aug 23, 2022
austinharris pushed a commit to austinharris/home-manager that referenced this pull request Dec 23, 2022
spacekookie pushed a commit to spacekookie/home-manager that referenced this pull request Feb 10, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.