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

Make release branches a thing #1259

Merged
merged 6 commits into from
Jan 14, 2025
Merged

Conversation

emilazy
Copy link
Collaborator

@emilazy emilazy commented Jan 8, 2025

Finally!

Once this is approved, I will cut a nix-darwin-24.11 branch and PR the appropriate changes.

Left as future work:

  • Building a seperate manual for each release, moving content from README.md and CHANGELOG to the manual, and linking an index page with the manuals for each version from README.md.

  • Infrastructure for automated backports.

  • Writing a checklist for the release process (or better, automating it).

  • Perhaps having a bot automatically update our flake.lock so we can just tell people to use nix-darwin’s Nixpkgs input rather than bringing their own (and risking mismatches).

Closes: #727

Comment on lines 62 to 67
system.darwinRelease = mkOption {
type = types.str;
default = (lib.importJSON ../../version.json).release;
description = "The nix-darwin release (e.g. `24.11`).";
};
Copy link
Contributor

Choose a reason for hiding this comment

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

This should be read-only, right?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this equivalent to a NixOS option?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yes, system.nixos.release, which is read only (so I’ve fixed that). The inconsistent naming here is unfortunate, but I thought it better to stick with the existing convention throughout the file and address all of them at once at some later date. (Though I do admit to feeling a little bad about introducing a new option with a name I don’t love.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

It would be good to just add a comment to document the equivalent option

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is just another entry in the various version options (system.darwinLabel, system.darwinVersion, system.darwinVersionSuffix, etc.) that all mirror various NixOS options. Do you want comments on all of those too? Maybe it should be in a different PR?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Sure a different PR is fine

@@ -3,7 +3,7 @@

inputs = {
nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
nix-darwin.url = "github:LnL7/nix-darwin";
nix-darwin.url = "github:LnL7/nix-darwin/master";
Copy link
Contributor

Choose a reason for hiding this comment

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

Hello,
Why this change? Isn't it already the default 🤔

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah. The eval-config.nix error tells people to ensure their input looks like github:LnL7/nix-darwin/NIX-DARWIN-BRANCH, and it felt simpler to just align the examples with that. I don’t love how it looks but I couldn’t think of a way to account for it in the eval-config.nix error that wouldn’t confuse people or be even more verbose. But I’m open to alternative suggestions.

My ideal would be to rename our master branch to nix-darwin-unstable to match nixpkgs-unstable, but that requires repo admin privileges so it’s not easy to do immediately.

Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like that it's more explicit, not sure how I feel about changing our default branch to nix-darwin-unstable as I don't think we really need the master and unstable split like nixpkgs

Comment on lines 62 to 67
system.darwinRelease = mkOption {
type = types.str;
default = (lib.importJSON ../../version.json).release;
description = "The nix-darwin release (e.g. `24.11`).";
};
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this equivalent to a NixOS option?

@@ -3,7 +3,7 @@

inputs = {
nixpkgs.url = "github:NixOS/nixpkgs/nixpkgs-unstable";
nix-darwin.url = "github:LnL7/nix-darwin";
nix-darwin.url = "github:LnL7/nix-darwin/master";
Copy link
Collaborator

Choose a reason for hiding this comment

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

I do like that it's more explicit, not sure how I feel about changing our default branch to nix-darwin-unstable as I don't think we really need the master and unstable split like nixpkgs

eval-config.nix Show resolved Hide resolved
eval-config.nix Show resolved Hide resolved
eval-config.nix Outdated Show resolved Hide resolved
eval-config.nix Outdated
Comment on lines 57 to 59
If `darwin` or `nixpkgs` are present in `nix-channel --list` (without
`sudo`), you should delete them with `nix-channel --remove NAME`.
These can contribute to version mismatch problems.
Copy link
Collaborator

Choose a reason for hiding this comment

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

I think this should live in its own check

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

As in, add an additional check to make sure the channels are all in order? I think we need to mention it here because having multiple nixpkgs channels that are out of sync with versions is a common source of issues; otherwise people might update one of the channels and still run into the error without ever being able to get to the check that would tell them what’s wrong.

I’m not opposed to having a separate check for this that triggers even if the versions are all in order but it should maybe be a separate PR? This will become a bigger deal as I push out stage 1 of The Plan, since people will be running darwin-rebuild as root and (I think? not sure) not have access to their regular user’s channels any more, especially since our installation instructions previously recommended adding darwin to the user’s channels (while nixpkgs gets automatically added to root’s channels). But in that case maybe the check should be in darwin-rebuild itself.

(The version check itself is deliberately outside of the module system and as early as possible in evaluation, because there can be changes to Nixpkgs that would prevent an entire configuration from evaluating and building in the first place.)

Copy link
Collaborator

Choose a reason for hiding this comment

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

Yeah, I was referring to checking that the channels are owned by root not by the current user

I think we should try to avoid additional nix-darwin specific login in darwin-rebuild, instead we could put it in the apply script as a check just to ensure that root has nixpkgs channel and that no other users have a nixpkgs channel

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I don’t think it could live in an apply script, because the problem I anticipate is sudo darwin-rebuild not being able to find the darwin channel to build the configuration at all, thanks to our historical installation instructions. But I’ll have to do some testing. Anyway, seems more in scope for the The Plan PR than this one?

.github/workflows/test.yml Outdated Show resolved Hide resolved
.github/workflows/test.yml Outdated Show resolved Hide resolved
Enzime
Enzime previously approved these changes Jan 14, 2025
Copy link
Collaborator

@Enzime Enzime left a comment

Choose a reason for hiding this comment

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

LGTM

@emilazy
Copy link
Collaborator Author

emilazy commented Jan 14, 2025

Here goes nothing.

@emilazy emilazy merged commit bd92122 into LnL7:master Jan 14, 2025
3 checks passed
@emilazy emilazy deleted the push-xztopxsynztu branch January 14, 2025 01:49
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.

RFC: Start using stable branches tied to Nixpkgs releases
4 participants