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

[RFC 140] Simple package paths, part 1b: Enabling the directory structure #237439

Merged
merged 3 commits into from
Sep 5, 2023

Conversation

infinisil
Copy link
Member

@infinisil infinisil commented Jun 12, 2023

Note
This was reviewed by and merged on 2023-09-05 15:30 CEST with the Nixpkgs Architecture Team and others that joined the call!

Context

This is the first part of the implementation of the accepted RFC 140, the introduction of the new directory structure. Note that this was split up into Part 1a (#250885) and Part 1b (this PR). The second part will be done afterwards, it's draft can be found here: #211832. This is hopefully only the first of many quality-of-life improvements to Nixpkgs by the Nixpkgs Architecture Team.

Overview

This RFC introduces the new pkgs/by-name directory, which makes it possible to add a new top-level package by just declaring it in pkgs/by-name/${prefix}/${name}/package.nix, where name is the attribute name the package should be exposed as, and prefix is the lowercased two-letter prefix of the attribute name. This means that for a lot of packages there's no more need to find an appropriate location for the file in the loosely categorized pkgs/ hierarchy, and no more editing of the all-packages.nix file!

Restrictions

Note however that only a subset of packages may be using this new directory structure for now, extending this is future work for the Nixpkgs Architecture Team. In particular these requirements have to be met for an attribute to be defined in pkgs/by-name:

  • It's at the top level, not allowed are packages from package sets like pkgs.python3Packages.requests.
  • It's a derivation, not allowed are non-derivations like pkgs.fetchFromGitHub.
  • It's defined using callPackage, not allowed are packages defined using functions like python3Packages.buildPythonApplication.

Things done

  • Implement the automatic calling of packages in the pkgs/by-name directory and move pkgs.hello to it
    • Working
    • Optimised
    • Tested
    • Clean and documented code
  • Implement CI checks enforcing the structure and semantics of the pkgs/by-name directory
    • Working, you can run nix-build -A tests.byName.nixpkgs
    • Is getting run by CI (see the checks in this PR)
      • Invalid pkgs/by-name breaks CI (example)
    • Tested, you can run nix-build -A tests.byName.tests
    • Clean and documented code
  • Update the contributor documentation for the pkgs/by-name directory and recommend using it for new packages
  • Add myself to relevant code owners entries

@infinisil infinisil added the significant Novel ideas, large API changes, notable refactorings, issues with RFC potential, etc. label Jun 12, 2023
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/quick-start.chapter.md Outdated Show resolved Hide resolved
pkgs/top-level/stage.nix Outdated Show resolved Hide resolved
pkgs/top-level/unit-files.nix Outdated Show resolved Hide resolved
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-06-13-nixpkgs-architecture-team-meeting-40/29087/1

pkgs/top-level/stage.nix Outdated Show resolved Hide resolved
.github/CODEOWNERS Outdated Show resolved Hide resolved
Copy link
Member

@gilice gilice left a comment

Choose a reason for hiding this comment

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

I am not sure you knew you can interpolate format strings in rust :)

Left diffs if you want to do it this way, but because it's mostly stylistic, I won't push it if you don't like it

pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/reference/src/main.rs Outdated Show resolved Hide resolved
@infinisil
Copy link
Member Author

@gilice I did not know that, neat! Greatly appreciated, I barely know Rust :D

@ofborg ofborg bot added the 2.status: merge conflict This PR has merge conflicts with the target branch label Jul 12, 2023
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
doc/contributing/coding-conventions.chapter.md Outdated Show resolved Hide resolved
pkgs/top-level/stage.nix Outdated Show resolved Hide resolved
doc/contributing/file-structure.chapter.md Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/default.nix Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/default.nix Outdated Show resolved Hide resolved
pkgs/test/named-hierarchy/default.nix Outdated Show resolved Hide resolved
@infinisil infinisil changed the title [RFC 140] Simple package paths, part 1: Introducing the name-based package hierarchy [RFC 140] Simple package paths, part 1: Directory structure Jul 14, 2023
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-07-13-documentation-team-meeting-notes-63/30450/1

@oxij
Copy link
Member

oxij commented Jul 18, 2023

Related NixOS/rfcs#109 (comment).

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/2023-09-05-nixpkgs-architecture-team-meeting-43/32635/1

pkgs/by-name/README.md Outdated Show resolved Hide resolved
This introduces the `pkgs/by-name` directory as proposed by RFC 140.
Included are:
- The implementation to add packages defined in that directory to the
  top-level package scope
- Contributer documentation on how to add packages to it
- A GitHub Actions workflow to check the structure of it on all PRs
@infinisil infinisil dismissed piegamesde’s stale review September 5, 2023 14:21

Everything was addressed

Copy link
Member Author

@infinisil infinisil left a comment

Choose a reason for hiding this comment

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

As planned we looked at this together in today's meeting with the Nixpkgs Architecture Team (including @aakropotkin, @roberth, @DavHau, @phaer), and @fgaz, @quantenzitrone and @YorikSar.

Everything was looked at and minor documentation updates were done. Nobody had any complaints about this PR, so we'll go ahead with merging it! 🚀

@infinisil infinisil merged commit ad61076 into NixOS:master Sep 5, 2023
4 checks passed
@github-actions
Copy link
Contributor

github-actions bot commented Sep 5, 2023

Backport failed for release-23.05, because it was unable to cherry-pick the commit(s).

Please cherry-pick the changes locally.

git fetch origin release-23.05
git worktree add -d .worktree/backport-237439-to-release-23.05 origin/release-23.05
cd .worktree/backport-237439-to-release-23.05
git checkout -b backport-237439-to-release-23.05
ancref=$(git merge-base f67a4c3f12a72f474a4349f6255f4a0b24b805ff 77d50b03e4388f22e1f36a2621a9287a12a138be)
git cherry-pick -x $ancref..77d50b03e4388f22e1f36a2621a9287a12a138be

@infinisil
Copy link
Member Author

Backport PR, let's make sure this one doesn't have any problems before merging it though: #253442

figsoda added a commit to nix-community/nix-init that referenced this pull request Sep 8, 2023
@infinisil infinisil added this to the RFC 140 milestone Sep 11, 2023
@zeuner zeuner mentioned this pull request Sep 12, 2023
12 tasks
@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/nix-init-generate-nix-packages-from-urls-with-hash-prefetching-dependency-inference-license-detection-and-more/25035/18

@doronbehar
Copy link
Contributor

I'm getting the following error from CI on a new PR I opened, from the check-by-name.yml workflow:

  Error: fatal: couldn't find remote ref refs/pull/255902/merge
  The process '/usr/bin/git' failed with exit code 128

@infinisil
Copy link
Member Author

Thanks for the report! Opened #256756 to discuss this

Comment on lines +64 to +65
$ emacs pkgs/by-name/so/some-package/package.nix
$ git add pkgs/by-name/so/some-package/package.nix
Copy link
Member

Choose a reason for hiding this comment

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

IMO this should just be default.nix to follow the previous pattern. The argument for naming it package.nix is rather weak and just creates confusion since there is no real reason to name it different.

Copy link
Member Author

Choose a reason for hiding this comment

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

This is part of the RFC so we can't just change that, and you already started a discussion on this in NixOS/rfcs#140 (comment), let's keep it in one place

@nixos-discourse
Copy link

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/ci-will-soon-enforce-pkgs-by-name-for-new-packages/38098/1

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
Status: Done
Development

Successfully merging this pull request may close these issues.