doc: update toPythonApplication section according to RFC 140
#294988
Conversation
|
Marking
|
|
calling it an alternative doesn't seem right, isn't |
|
This format provides a more robust way to override dependent python packages, and also make the dependent
I also think this should be considered the preferred way, while the current hard-coded one-liners in However, we do hope to maintain a consistent overriding interface. Considering the following scenario: { python3Packages }:
with python3Packages; toPythonApplication youtube-dlwhere |
|
i don't actually know python, so i'm not much help here, but if such a fundamental issue as "how do we package for different python versions" isn't solved yet, then it sounds like this packaging method isn't ready. |
|
BTW, why do we prefer |
I personally prefer the second approach, i.e. pin the version in |
|
I think using
This is indeed a concern, and I agree with your conclusion:
This is also what is recommended in |
infinisil
added
the
architecture
Splicing dark Magick. |
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
from
411766d
to
455726a
Compare
|
Ops, my failure! |
|
(Discussed in the docs team meeting today)
I think it's fine for something to be recommended even if older instances don't use the pattern (yet). |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/2024-03-14-documentation-team-meeting-notes-113/41462/1 |
| Since the advent of `pkgs/by-name` hierarchy, there is an extra alternative: | ||
| instead of writing the code above in `all-packages.nix`, the corresponding code |
There was a problem hiding this comment.
Given the agreement that this is the preferred way to declare these types of packages going forward, can you reword this to fit in the previous paragraph (instead of "A reference shall be created...", reword to fit this text there instead)?
And then we can ellaborate on:
- Existing packages and why they must remain in
all-packages.nix. I'd go with a note first (:::{.note}), but if the text becomes too large, can be a subsection. - What to do in cases we need to override the package
There was a problem hiding this comment.
- Unfortunately the override can be a bit complicated, since the "library" is accessed behind
python3Packages. But it happens in both scenarios, so nothing was lost. - I am sprint-removing the ones in top-level.
That being said, I believe I will need to rewrite this whole text, showing the new vs the legacy method or something similar.
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
2 times, most recently
from
3fed991
to
f2021fd
Compare
|
@DanielSidhion please review again. |
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
from
f2021fd
to
1daf4cf
Compare
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
from
1daf4cf
to
f59765b
Compare
|
|
||
| with python3Packages; toPythonApplication youtube-dl | ||
| ``` | ||
|
|
There was a problem hiding this comment.
Document the way to ping the Python version of a Python application exposed this way for override interface consistency. (Feel free to rephrase it.)
| In case the top-level Python application requires a specific interpreter version, specify the canonical `python3Packages` to use in `all-packages.nix`, instead of hard-coding the desired `python3*Packages`, to keep the [override](#sec-pkg-override) interface consistent. | |
| ::: {.example #ex-topythonapplication-by-name-ping} | |
| # Ping the interpreter version of a Python application exposed in `pkgs/by-name`. | |
| ```nix | |
| { | |
| youtube-dl = callPackage ../by-name/yo/youtube-dl/package.nix { | |
| python3Packages = python312Packages; | |
| }; | |
| } | |
| ``` | |
| ::: |
There was a problem hiding this comment.
This is already documented in the pkgs/by-name readme. I thought about making the same suggestion, but IMO it's better to have that information deduplicated. Perhaps add a pointer to the readme instead?
There was a problem hiding this comment.
How about
| In case the top-level Python application requires a specific interpreter version, [change the implicit default value](https://github.com/NixOS/nixpkgs/tree/master/pkgs/by-name#changing-implicit-attribute-defaults) of argument `python3Packages` to the desired package set, instead of hard-coding `python3*Packages` as an argument. This ensures consistent interpreter version overriding via [`<pkg>.override](#sec-pkg-override) and argument `python3Package`. |
There was a problem hiding this comment.
Some minor suggestions here:
- We should probably avoid prone-to-irrelevance naming like "legacy method" and "new method", especially for document anchors, which are hard to change without breaking hyperlinks.
- Use example blocks to make the examples more discoverable and linkable.
| hierarchy, in a `callPackage` fashion. Taking the above `youtube-dl` as an | ||
| example, the reference is registered at | ||
| `pkgs/by-name/yo/youtube-dl/package.nix`: | ||
|
|
There was a problem hiding this comment.
| ::: {.example #ex-topythonapplication-function-by-name} | |
| # Convert to a Python application and register in `pkgs/by-name` |
| { python3Packages }: | ||
|
|
||
| with python3Packages; toPythonApplication youtube-dl | ||
| ``` |
There was a problem hiding this comment.
| ``` | |
| ``` | |
| ::: |
| the new method instead. | ||
| ::: | ||
|
|
||
| ##### New Method {#topythonapplication-function-new-method} |
There was a problem hiding this comment.
| ##### New Method {#topythonapplication-function-new-method} | |
| ##### Convert and register under name-based package directories {#topythonapplication-function-by-name} |
My intention is to remove the legacy reference as soon as possible (right after converting the code), however it requires some testing.
Good point. |
Sounds great! How about introducing the new method directly, with a note block talking about the migration? The existing approach could be mentioned as Prior to the [name-based package directories](https://github.com/NixOS/nixpkgs/blob/master/pkgs/by-name/README.md),
the Python-application reference to a Python module was placed inside `all-packages.nix`. |
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
2 times, most recently
from
226316f
to
4a38344
Compare
AndersonTorres
changed the title
toPythonApplication stoPythonApplication section according to RFC 140
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
2 times, most recently
from
b631b85
to
445f2a4
Compare
|
My laptop is awaiting repair, and I might be slow to response until setting up the machine I borrowed. Thank you for your patience.
Does that mean there will be a bulk migration from the legacy reference approach to the new one? |
Yes: |
The advent of `pkgs/by-name` hierarchy created an interesting method: instead of cluttering `all-packages.nix` with more lines and more merge conflicts, the creation of `toPythonApplication` expressions can be made at `pkgs/by-name`. This method should be the default for new packages, while the old one should be phased out.
AndersonTorres
force-pushed
the
topythonapp-update-docs
branch
from
445f2a4
to
913bc71
Compare
|
The new "note about migration" looks great! Do we need to remind people to follow the "changing the implicit default value" guide of name-based package directory when using a specific Python version? |
| with python3Packages; toPythonApplication youtube-dl | ||
| ``` | ||
|
|
||
| Sometimes, the top-level Python application requires a specific Python |
There was a problem hiding this comment.
Do we need to remind people to follow the "changing the implicit default value" guide of name-based package directory when using a specific Python version?
I have done it here, line 377
The advent of
pkgs/by-namehierarchy created an interesting alternative: instead of clutteringall-packages.nixwith more lines and more merge conflicts, the creation oftoPythonApplicationexpressions can be made atpkgs/by-name.Description of changes
Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 馃憤 reaction to pull requests you find important.