doc/python: document fixed-point arguments (#271387)#477149
doc/python: document fixed-point arguments (#271387)#477149doronbehar wants to merge 4 commits intoNixOS:masterfrom
Conversation
5ad3d8b to
99403d3
Compare
99403d3 to
5339e49
Compare
|
Thanks for working on the docs! I had assumed that fixpoint-args support was deliberately undocumented until support was fully backported to all supported versions (currently just 26.05 and 25.11) and had some time to incubate. This prevents confusion when someone reads the unstable docs and assumes that they apply equally to stable Nixpkgs. I'll defer to @ShamrockLee w.r.t. when we're ready to publicly document fixpoint-args support. |
| pname = "pyspread"; | ||
| inherit (finalAttrs) version; |
There was a problem hiding this comment.
I'd expect it to inherit pname as well, this is one of the few times I'd think using pname would be fine (as opposed to e.g: fetchFromGitHub's repo argument).
There was a problem hiding this comment.
It isn't really. We normalize pname (e.g. apply lowercasing), but the pypi name may not reflect that, so it is often untied.
There was a problem hiding this comment.
it is often untied
So this means while this documentation suggest untying it, tying pname is also accepted?
There was a problem hiding this comment.
Yeah, it might be stable, if the library name already is normalized, but uppercase, dashes, etc. will cause flapping every once in a while.
There was a problem hiding this comment.
Even if you do reuse the name for pname and src.pname, it's best to use a locally defined variable rather than finalAttrs.pname. Values in finalAttrs represent the final state, including any user defined overrides.
pname is an example of something an end-user may wish to override, e.g. pname = prevAttrs.pname + "-with-my-tweaks". Such an override shouldn't propagate to the src's project.
|
Thanks for the suggestion @Sigmanificient , I'm not sure about the full list of those, so I wrote only |
|
@ShamrockLee might be able to give you a proper list |
This isn't just some args being passthru attrs instead of derivation attrs, it's also some args being entirely renamed to other attrnames. The most prominent example is renaming everything That is already documented under Taking a step back, I don't think it's practical to list every single attr that is or isn't accessed via passthru2 and every single attr that gets transformed or renamed when calling the underlying Footnotes
|
You can find them by seeing both As GitHub mobile is suffering from https://github.com/orgs/community/discussions/181590 , could someone help me start a review comment thread targeting lines from |
| NOTE: The following attributes, are not available in `finalAttrs`, but are available in `finalAttrs.passthru`: | ||
|
|
||
| - `format` | ||
| - `optional-dependencies` |
There was a problem hiding this comment.
@ShamrockLee please suggest something different here.
There was a problem hiding this comment.
| NOTE: The following attributes, are not available in `finalAttrs`, but are available in `finalAttrs.passthru`: | |
| - `format` | |
| - `optional-dependencies` | |
| ::: {.note} | |
| Some `buildPythonPackage`/`buildPythonApplication` arguments are passed down to `stdenv.mkDerivation` via `passthru` instead of passing directly. | |
| Their final states can be accessed via `finalAttrs.passthru.${name}`, and they can be overridden via [`<pkg>.overrideAttrs`](#sec-pkg-overrideAttrs) under the `passthru` attribute. | |
| Such arguments include: | |
| - `disabled` | |
| - `pyproject` | |
| - `build-system`, `dependencies` and `optional-dependencies` | |
| ::: |
There was a problem hiding this comment.
This looks good to me, @MattSturgeon what do you think?
There was a problem hiding this comment.
As always, we could polish the wording a little. But, yes it LGTM.
I still feel like a followup PR may be able to consolidate some of the "be aware: the args you pass to buildPythonPackage are not the same as what's in finalAttrs" documentation, as per my rambling in #477149 (comment)
There was a problem hiding this comment.
As always, we could polish the wording a little. But, yes it LGTM.
I reworded it a little bit, I hope it is better.
I still feel like a followup PR may be able to consolidate some of the "be aware: the args you pass to
buildPythonPackageare not the same as what's infinalAttrs" documentation, as per my rambling in #477149 (comment)
I agree, but I feel this topic is going under substantial changes and it might be better to document this subject once we are satisfied with the behavior :). See also my <!-- TODO --> comment there.
There was a problem hiding this comment.
Seems reasonable overall 👍🏻
If I'm nitpicking,
Don't worry about it, I'm patient and your comments are great 👍 .
it feels odd to explicitly mention
overrideAttrshere:Therefore the final states of these attributes can be accessed via
finalAttrs.passthru.${name}, and they can be overridden via<pkg>.overrideAttrsunder thepassthruattribute....
overrideAttrsuses the samefinalAttrsfixpoint arg asmkDerivation(and by extensionbuildPythonPackage), so calling it out explicitly with an "and" feels like we're implying it does things differently.That said, I get where you're coming from. It's probably helpful to highlight that this "
finalAttrsis different to the initialargs" quirk also applies inoverrideAttrs.
TBH I'm not sure I fully understood how you understood the docs I read :). I think when @ShamrockLee chose to explicitly mention overrideAttrs they did that to imply to use .overrideAttrs and not .overridePythonAttrs, since the latter doesn't support the signature (finalAttrs: prevAttrs: {}). I tried to do that while not shaming ourselves that this feature is not yet supported due to #258246 :). The TODO comment below is the comment that should remind us to update that text once #258246 is resolved.
Maybe we can be optimistic that #258246 will be resolved soon and we can postpone this .note phrasing to afterwards? There were other voices who wanted to hold back this PR a little bit to let this new exciting feature stabilize.
There was a problem hiding this comment.
How about:
Their final states can be accessed via `finalAttrs.passthru.${name}`, or overridden via [`<pkg>.overrideAttrs`](#sec-pkg-overrideAttrs) in the same way.The TODO comment below is the comment that should remind us to update that text once #258246 is resolved.
Personally, I'm in favour of adding finalAttrs support to overridePythonAttrs. It's actually a trivial change (I had a patch somewhere, but I can't find it to link).
I think the main argument against doing so is the desire to deprecate overridePythonAttrs entirely, in favor of exclusively using overrideAttrs:
As for
overridePythonAttrs, it should be deprecated afteroverrideAttrsfor Python packages is ready.
-- #258246 (comment)
I personally hope to push
overrideAttrs, but recognize thatoverridePythonAttrsare still widely used and that Python packages'overrideAttrsinterface still has some unsettled parts (i.e., thecheck/installCheckattributes).
-- #258246 (comment)
Personally, I don't see the two ideas as mutually exclusive; we could add fixpoint-arg support now and still deprecate the function later, if we wanted to.
On the basis that its future direction is uncertain, I'm on the fence w.r.t. including the TODO comment, but I won't block on it 🙂
There were other voices who wanted to hold back this PR a little bit to let this new exciting feature stabilize.
I think that was just me justifying why we initially merged fixpoint-args support without new docs. Once @ShamrockLee is happy for this to be documented, I am too.
There was a problem hiding this comment.
I think the main argument against doing so is the desire to deprecate
overridePythonAttrsentirely, in favor of exclusively usingoverrideAttrs:
OK I see, I wasn't aware of that since I haven't read thoroughly all of #258246. I have revised the TODO comment a bit to reflect that.
There was a problem hiding this comment.
I personally hope to push
overrideAttrs, but recognize thatoverridePythonAttrsare still widely used and that Python packages'overrideAttrsinterface still has some unsettled parts (i.e., thecheck/installCheckattributes).
-- #258246 (comment)
@doronbehar Could you please help me recover the content of the quoted comment from your mailbox? GitHub Mobile tricked me into deleting it.
fe37bc4 to
bd1e678
Compare
bd1e678 to
ad54fe3
Compare
Co-authored-by: Matt Sturgeon <matt@sturgeon.me.uk>
424a126 to
5ac2996
Compare

Things done
passthru.tests.nixpkgs-reviewon this PR. See nixpkgs-review usage../result/bin/.Add a 👍 reaction to pull requests you find important.