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.

Sign up for GitHub

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

init: devshell for hands-on onboarding #119966

Closed
wants to merge 3 commits into from
Closed

init: devshell for hands-on onboarding #119966

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
5044f8d
init: devshell for hands-on onboarding
Apr 20, 2021
b9bd449
devhsell: pin nixpkgs version
Apr 21, 2021
4c24484
devshell: use good old nix
Apr 21, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions flake.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -63,6 +63,11 @@
});

checks.x86_64-linux.tarball = jobs.tarball;
devShell = {
x86_64-linux = import ./shell.nix { system = "x86_64-linux"; };
x86_64-darwin = import ./shell.nix { system = "x86_64-darwin"; };
aarch64-linux = import ./shell.nix { system = "aarch64-linux"; };
Copy link
Member

Choose a reason for hiding this comment

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

Do we really have to manually list every platform here? What happens to somebody who isn't on one of those three platforms? (aarch64-darwin and powerpc64le-linux come to mind.)

Copy link
Contributor Author

@blaggacao blaggacao Apr 22, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

When writing I had a bad stomach feeling, already. 😉

numtide/flake-utils has a defaultSystem. Do we have something similar to enumerate on the "blessed" set of systems as flake spec requires?

Maybe we also could just add those two to suport 'most common' platforms? What do you think?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Somebody on a different platfrom currenrly still can try nix-shell directly using shell.nix circumventing flake.nix.

};

htmlDocs = {
nixpkgsManual = jobs.manual;
Expand Down
45 changes: 45 additions & 0 deletions shell.nix
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,45 @@
{ system ? builtins.currentSystem }:
Copy link
Member

Choose a reason for hiding this comment

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

Is this file supposed to only use builtins?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, it shall have no other dependency.

I'd rather avoid public interfaces, where we can. Everything needed will be conveniently contained within devshell and nixpkgs.

let
# nixpkgs / devshell is only used for development. Don't add it to the flake.lock.
nixpkgsGitRev = "5268ee2ebacbc73875be42d71e60c2b5c1b5a1c7";
devshellGitRev = "709fe4d04a9101c9d224ad83f73416dce71baf21";

nixpkgsSrc = fetchTarball {
url = "https://github.com/NixOS/nixpkgs/archive/${nixpkgsGitRev}.tar.gz";
sha256 = "080fvmg0i6z01h6adddfrjp1bbbjhhqk32ks6ch9gv689645ccfq";
};


devshellSrc = fetchTarball {
Copy link
Member

Choose a reason for hiding this comment

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

Flakes shouldn't use `builtins.{fetchTarball,fetchGit,...}. This may even become impossible in the future. Dependencies should be expressed at the flake level so they can be queried and updated in a uniform way.

Copy link
Contributor Author

@blaggacao blaggacao Apr 21, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

Thanks for pointing this out! We had a large discussion over at divnix/devos if this is an antipattern or actually a good pattern. Most lock files, support some notion of development dependencies and "usage" dependency (for not to say "runtime").

For example flake-utils-plus like numtide/flake-utils has a policy to depend on nothing, but builtins. This makes a lot of sense so that no subtle incompatibilities would be introduced since coded against one nixpkgs version it could be instantiated with another.

Importing those dependencies via flake inputs, for those packages it would be absolutely out of question to provide such development amentities. Until flakes support dev-only (optional) dependencies, which are not required for using a flake, this is the only pattern we could find to map this use case concisely into code.

Would you be ok to remain explicit in a sense that once flake supports that, we can change it, or would you still prefer to bury that subtle distinction and make it explicit once flake's support it? I don't have any vested preferences here.

url = "https://github.com/numtide/devshell/archive/${devshellGitRev}.tar.gz";
sha256 = "1px9cqfshfqs1b7ypyxch3s3ymr4xgycy1krrcg7b97rmmszvsqr";
};

pkgs = import nixpkgsSrc { inherit system; };
devshell = import devshellSrc { inherit system pkgs; };
Copy link
Contributor

Choose a reason for hiding this comment

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

I see a fundamental problem here: There is no way to introduce breaking changes in nixpkgs that touch devshell without patching the latter along. Otherwise the shell will simply break. We just added quite an arbitrary dependency.

Technically correct solution would be to pass the historic version of nixpkgs that devshell was developed against, but that's quite expensive.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Technically correct solution would be to pass the historic version of nixpkgs that devshell was developed against, but that's quite expensive.

I can see the problem. For the reasonable future, I think devshell will be well maintained and any breakage will be resolved within days, not weeks.

The problem manifests once/if (core) maintainers or the CI becomes heavily dependent on it. By then, devshell, whose stated objective is to completment mkShell here in nixpkgs, will either be already upstreamed or heavily maintained.

Historical nixpkgs, without further guardrails, would imply historical nixpkgs-fmt, historical nixpkgs-review, etc, etc.

I think cost benefit tips slightly towards accepting the risk that it (might evnetually maybe) break. ... and fix it when we get there.


in
devshell.mkShell {
devshell = {
name = "nixpkgs";
packages = with pkgs; [
fd
nixpkgs-fmt
];
};

commands = [
{
name = "fmt";
help = "Check Nix formatting";
category = "linters";
command = "nixpkgs-fmt $${@} .";
}
{
name = "evalnix";
help = "Check Nix parsing";
category = "linters";
command = "fd --extension nix --exec nix-instantiate --parse --quiet {} >/dev/null";
}
];
}