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

Switch from poetry to nixpkgs #515

Merged
merged 6 commits into from
Oct 16, 2023

Conversation

zmitchell
Copy link
Contributor

This PR removes the poetry based workflow in favor of a nixpkgs based workflow. In doing so the Python version will be updated from 3.7 to 3.10. This also necessitates upgrading sphinx to v5.1.1. The version of sphinx-book-theme in nixpkgs is v0.3.3, which is not compatible with sphinx v5.x. The latest version of sphinx-book-theme is v1.0.1, which is compatible with sphinx v5.x, but it's not in nixpkgs.

@zmitchell zmitchell mentioned this pull request Apr 7, 2023
2 tasks
flake.nix Outdated
name = "nix-dev";
src = self;
buildInputs = with pkgs.python310Packages; [
livereload
Copy link
Member

Choose a reason for hiding this comment

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

livereload is only used for local development as it is a tool which invokes a build command every time a change is detected.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Are you suggesting I do something differently?

Copy link
Member

Choose a reason for hiding this comment

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

@zmitchell Yeah, I'm just saying it's not necessary to be inside buildInputs since it is not a dependency required for building the website.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, that part I get. What I don't get is where black and livereload are supposed to go instead. The live script relies on livereload being available for the call to nix-shell --run, and since I'm new to Nix I'm not sure the best way to achieve the same thing if those Python modules aren't in buildInputs.

Copy link
Member

Choose a reason for hiding this comment

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

A good approach is to have the main derivation describing only the tools and libraries it needs to build, and then have a separate derivation corresponding to a development shell where you add additional tools.

flake.nix Outdated
sphinx-book-theme
sphinx-copybutton
sphinx-design
black
Copy link
Member

Choose a reason for hiding this comment

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

black is also a development only tool as it is a Python linter.

@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-04-13-learning-journey-working-group-meeting-notes-4/27255/1

@fricklerhandwerk
Copy link
Collaborator

Discussed in the Nix documentation team meeting:

  • @zmitchell: How to run the ./live script with flakes?

@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-04-27-documentation-team-meeting-notes-44/27694/1

flake.nix Show resolved Hide resolved
flake.nix Outdated
defaultPackage = pkgs.stdenv.mkDerivation {
name = "nix-dev";
src = self;
buildInputs = with pkgs.python310Packages; [
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
buildInputs = with pkgs.python310Packages; [
nativeBuildInputs = with pkgs.python310Packages; [

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Can you explain the motivation here? I gather that (native|propagated)BuildInputs and buildInputs are rooted in cross compilation, but this distinction is lost on me in the context of Python.

Copy link
Member

Choose a reason for hiding this comment

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

Simply correctness; native build inputs are your tools. Yes, outside cross-compilation it is less valuable however for Python it for sure still matters. Remember, there is a lot of native code used by Python modules.

Note buildPython* makes the distinction (it sets strictDeps = true;).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Let me rephrase the question to hone in on the part that confuses me: in general and in the Python specific case, when should I put packages in buildInputs, nativeBuildInputs, or propagatedBuildInputs?

Copy link
Member

Choose a reason for hiding this comment

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

  • nativeBuildInputs: these are build-time tools, that is, programs that run during the build (hence they need to be native)
  • buildInputs: these are headers/libraries which one needs during build time, e.g. to link against, but it is not executed. These are thus your run-time dependencies.
  • propagatedBuildInputs: these are buildInputs but that are propagated, that is, whoever adds your derivation in a buildInputs, will automatically get all these propagatedBuildInputs as well. And this is done recursively.

The Nixpkgs manual describes this in more detail.

In buildPython* we use propagatedBuildInputs for our run-time dependencies, which in a way is in line with what I wrote above, but it has its issues. It's a historic artifact and unfortunately a complete pain to fix.

flake.nix 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-10-05-documentation-team-meeting-notes-84/33877/1

@github-actions
Copy link
Contributor

Deploy Preview

Name Result
Last commit: c8667ac7a711c208470ee20b29b6e5a7a8b2c88e
Preview URL: https://43c42c08.nix-dot-dev.pages.dev

@github-actions
Copy link
Contributor

Deploy Preview

Name Result
Last commit: 4da4d1885829f61d6b0747d66cdb88f977e77638
Preview URL: https://f938251b.nix-dot-dev.pages.dev

@alejandrosame alejandrosame marked this pull request as ready for review October 10, 2023 19:29
@alejandrosame alejandrosame requested a review from a team as a code owner October 10, 2023 19:29
Comment on lines +12 to +16
overlays = [
# Add sphinx-sitemap from an overlay until
# it becomes available from nixpkgs-unstable
(import ./overlay.nix)
];
Copy link
Member

Choose a reason for hiding this comment

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

sphinx-sitemap PR: NixOS/nixpkgs#260274

@github-actions
Copy link
Contributor

Deploy Preview

Name Result
Last commit: 32b0aa0fe80f1406894ba650d4b3935f913d292f
Preview URL: https://989730fb.nix-dot-dev.pages.dev

@alejandrosame
Copy link
Member

PR to add sphinx-sitemap to nixpkgs: NixOS/nixpkgs#260274

@fricklerhandwerk fricklerhandwerk merged commit 8c657ff into NixOS:master Oct 16, 2023
4 checks passed
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.

6 participants