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

Pin Nixpkgs (without Flakes) #405

Merged
merged 14 commits into from Aug 31, 2021
Merged

Conversation

samueldr
Copy link
Member

@samueldr samueldr commented Aug 26, 2021

This does not replace nor obsoletes #404. In fact, #404 should be rebased on top of this to better synchronize the non-flake and flake-based implementation of the project.


What?

The TODO item about pinning Nixpkgs has long been put on the backburner; a hole was burned at the bottom of the pot.

This changes the default behaviour of nix-build in the repository; instead of relying on the ambiant <nixpkgs> it uses a known good revision of Nixpkgs.

Overriding to use your own <nixpkgs> can still be done using --arg pkgs 'import <nixpkgs> {}'. It should help reduce the friction of first builds for end-users.

This does not affect use inside a system configuration:

{
  imports = [
    (import <mobile-nixos/lib/configuration.nix> { device = "xxx-yyy"; })
  ];
}

The Nixpkgs used is whatever the NixOS modules system resolved, just as it would already.

But Flakes?

What about Flakes? This does not replace usage of Nix Flakes. It uses the pinning mechanism for better cooperation for once we have a good user-story for Nix Flakes.

So unless I'm mistaken, merging this changes absolutely nothing with regards to Flakes. Whether it be current Flakes users, or for a future use of Flakes.

Note: The first draft used a bogus flake.lock to pin the Nixpkgs revision, the current one does not. It changes nothing in the end; the only difference is that in #404 pkgs.nix will need to use the actual data from the flake.nix/flake.lock pair, which is out of scope in this PR.

But hermeticity?

Flakes are not the only way to add hermeticity to a Nix build. Though the builds are still not hermetic by default! The "default" build still reads the ambiant environment for the current system. Convenience here is worth the minor loss of hermeticity; it is made obvious through tracing that cross-compilation is automatically being used, and there is a known way to undo that "hole" if the user desires it.

And release.nix?

It is by design that the Hydra-consumed release.nix still relies on NIX_PATH for <nixpkgs>. Assumptions are made that Hydra evaluations are declared correctly enough to be functionally equivalent to totally hermetic.

@samueldr
Copy link
Member Author

Oops, I forgot shell.nix, which was the whole point of it...

@zhaofengli
Copy link
Member

It's weird to have a flake.lock without a flake since it's generated by Nix based on the original flake. There is no good story on updating it without creating a temporary flake and running the tools, so let's not do that at all.

@samueldr
Copy link
Member Author

samueldr commented Aug 26, 2021

I would tend to agree, but this is where #404 comes in, tying the loose end.

When I had it designed in my mind a few weeks back it wasn't going to rely on flake.lock at all.

@zhaofengli
Copy link
Member

I was under the impression that you are not intending to merge it anytime soon. From #404:

Additionally, until it is enabled by default as part of a stable Nix release I'm not comfortable with adding Flake support.

@samueldr
Copy link
Member Author

samueldr commented Aug 26, 2021

¯\_(ツ)_/¯

I can always revert back to not using flake.lock; but instead to lock by adding the rev/hash in pkgs.nix.

@samueldr
Copy link
Member Author

And removed the old crufty overlay/default.nix!

@colemickens
Copy link
Member

Was just using flake-compat considered? That is how I utilize flakes without anyone being the wiser in various projects consumed by then community.

Aside, I know this isn't where you might want to spend energy but I've been treating this as a blocker and am excited to see the discussion. Sincere thanks to all.

@samueldr
Copy link
Member Author

samueldr commented Aug 27, 2021

How has the "lack" of Flakes been a blocker?

As far as I understand it, Mobile NixOS has been usable in a Flakes-based configuration since we got <nixpkgs> references excised from the configuration a good time ago. Leaving only references at entry points, which are now almost all removed. (There's only release.nix left, and that won't go away because of how the Hydra config works.)

Using this repository as-is without integrating in a NixOS configuration is only good to produce an unusable unconfigured image.

Or am I missing something still?

@ostylk
Copy link

ostylk commented Aug 27, 2021

The way you parse the flake.lock is a bit erronous. As soon as the user pins nixpkgs to a revision outside of GitHub it just fails. It would be definitely better to use flake-compat but that requires a flake.nix. pkgs.nix would look like this (usage stays the same):

import (import (fetchTarball https://github.com/edolstra/flake-compat/archive/master.tar.gz) {
  src = builtins.fetchGit ./.;
}).defaultNix.inputs.nixpkgs

Maybe just remove the flake.lock from this PR and pin some arbitrary nixpkgs commit in pkgs.nix, so you can cleanly merge the pinning mechanic to this repo. Then in #404 I'll replace pkgs.nix with the above version ensuring that the correct version of nixpkgs is used (even if the user pinned some weird non-github commit).

@samueldr samueldr added the 4. type: enhancement New feature or request label Aug 27, 2021
@samueldr samueldr changed the title Pin Nixpkgs based on flake.lock data (without Flakes) Pin Nixpkgs (without Flakes) Aug 27, 2021
@samueldr
Copy link
Member Author

samueldr commented Aug 27, 2021

Yeah, I got a recommendation from another user for using that; I didn't know enough about Flakes to know if it could break some use case.

When I had planned pinning Nixpkgs (without Flakes, still) it was with a "plain" import like I'm now doing.

I'm planning to merge this soon, unless people here see reasons not to.

One nice side-effect, as it is, is that we get pre-built artifacts from Hydra by default, so new users are less likely to need to build anything.

doc/default.nix Outdated
Comment on lines 4 to 13
let pkgs' = pkgs; in # Break the cycle
let
pkgs = import pkgs'.path {
inherit (pkgs') system config;
overlays = pkgs'.overlays ++ [
(final: super: {
mobile-nixos-process-doc = final.callPackage ./_support/converter {};
})
];
};
Copy link
Member Author

@samueldr samueldr Aug 31, 2021

Choose a reason for hiding this comment

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

shell.nix Outdated
Comment on lines 3 to 10
let pkgs' = pkgs; in # Break the cycle
let
pkgs = import pkgs'.path {
inherit (pkgs') system config;
overlays = pkgs'.overlays ++ [
(import ./overlay/overlay.nix)
];
};
Copy link
Member Author

@samueldr samueldr Aug 31, 2021

Choose a reason for hiding this comment

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

It wasn't really "nice" to use... Had no real value IMO.
@samueldr samueldr merged commit 796040d into NixOS:master Aug 31, 2021
@samueldr samueldr deleted the feature/pkgs-pinning branch August 31, 2021 18:35
@ryneeverett
Copy link
Contributor

I see there are still warnings to set NIX_PATH in the "Getting Started" and "Device Porting Guide" pages in the docs. Should these be purged?

@samueldr
Copy link
Member Author

Good catch; they should be removed. They're largely useless without providing --arg pkgs or doing other operations.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
4. type: enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants