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

allow rec in packaging functions #762

Closed
wants to merge 1 commit into from
Closed

allow rec in packaging functions #762

wants to merge 1 commit into from

Conversation

wegank
Copy link
Member

@wegank wegank commented Oct 18, 2023

The best practice on rec is often interpreted in nixpkgs PRs as "rewriting rec using let ... in fixes the overriding logic", whereas it isn't the case. The second pitfall is therefore less justified.

In the discussion about the same topic on Discourse, the most liked post also points out that avoiding rec in packaging functions doesn't make much sense.

Hence this PR.

@wegank wegank requested a review from a team as a code owner October 18, 2023 01:02
@github-actions
Copy link
Contributor

Deploy Preview

Name Result
Last commit: 0bf86078fb369658b4ae0005efb53cc7178a2ff4
Preview URL: https://b3b4dbf7.nix-dot-dev.pages.dev

@fricklerhandwerk
Copy link
Collaborator

fricklerhandwerk commented Oct 18, 2023

The correct resolution here would be recommending the finalAttrs: pattern, but that's not really well documented in the Nixpkgs manual yet and still only rarely used. @ShamrockLee has a Nixpkgs PR open that I'd like to get merged once it's in shape, but we could put something here on nix.dev in the interim. @infinisil @proofconstruction what do you think?

@proofconstruction
Copy link
Contributor

What situations exist where the finalAttrs pattern cannot be used? (Other than aesthetically)

@wegank
Copy link
Member Author

wegank commented Oct 19, 2023

As far as I know, the finalAttrs pattern is only supported by a few language helpers, such as buildNimPackage. For the most common ones, like buildGoModule, buildPythonPackage, or rustPlatform.buildRustPackage, the support isn't there.

@fricklerhandwerk
Copy link
Collaborator

fricklerhandwerk commented Oct 19, 2023

I'd prefer to recommend finalAttrs: directly (saying to use it where applicable), right there in the highlighted block. That would also be a good opportunity to place that link to the Nixpkgs manual so it's easier to find. All the details on using rec around overriding should go to the Nixpkgs manual. The overrides section would be a reasonable place. I want to keep nix.dev a thin guidance wrapper around reference documentation.

@SuperSandro2000
Copy link
Member

finalAttrs also has it pitfalls like FODs used in src where it changing rev breaks the FOD which is unnoticed because the original is probably in the cache.

@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-30-documentation-team-meeting-notes-90/34936/1

@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-11-02-documentation-team-meeting-notes-91/34938/1

@fricklerhandwerk
Copy link
Collaborator

@NixOS/documentation-team decided to avoid the confusion by not mentioning the overrideAttrs case, which is indeed unrelated to the best practice here.

Superceded by #790

Still, it needs better docs in Nixpkgs: NixOS/nixpkgs#266485

@wegank wegank deleted the rec-packaging-drop branch November 9, 2023 15:43
@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-11-09-documentation-team-meeting-notes-93/35244/1

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

Successfully merging this pull request may close these issues.

5 participants