Skip to content

Comments

Doc: pkgs{compressDrv,compressDrvWeb} improve clarity#332466

Merged
philiptaron merged 1 commit intoNixOS:masterfrom
hsjobeki:doc/compressDrv
Aug 5, 2024
Merged

Doc: pkgs{compressDrv,compressDrvWeb} improve clarity#332466
philiptaron merged 1 commit intoNixOS:masterfrom
hsjobeki:doc/compressDrv

Conversation

@hsjobeki
Copy link
Contributor

@hsjobeki hsjobeki commented Aug 5, 2024

Description of changes

Note: This doesnt get rendered into any of the manuals:
The only place where people can discover it currently is the code and https://noogle.dev/f/pkgs/compressDrv

  • Improves clarity
  • More consistent with the other docs.
  • Move the extensive example into an example block, which can be collapsed / is collapsed by default in nixpkgs manual.

cc: @motiejus @Ma27

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.11 Release Notes (or backporting 23.11 and 24.05 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 5, 2024

@motiejus Somwething i missed in the initial pr:

Why do you need ... ?
{ formats, compressors, ... }:

It is not possible to access the 'rest' arguments, and they are not forwarded to anything. Thus calling the function, with superfluous arguments will not result in an error.

@motiejus
Copy link
Contributor

motiejus commented Aug 5, 2024

This looks great, but I can only judge on the surface. Is there a simple-ish way to generate something like https://noogle.dev/f/pkgs/compressDrv locally to review rendered changes?

@motiejus Somwething i missed in the initial pr:

Why do you need ... ? { formats, compressors, ... }:

It is not possible to access the 'rest' arguments, and they are not forwarded to anything. Thus calling the function, with superfluous arguments will not result in an error.

This is a left-over from previous PR incarnations and can be removed. You can do it here (AFAIK), or I can do this once this PR is merged -- I let's avoid conflicts either way.

@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux. labels Aug 5, 2024
@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 5, 2024

This looks great, but I can only judge on the surface. Is there a simple-ish way to generate something like https://noogle.dev/f/pkgs/compressDrv locally to review rendered changes?

No unfortunately Not. Usually you would just build the Manual. But this doesnt appear in the Manual yet.

Rendering this into the Manual is a Goal but a little complex, since we want a generic solution.

You could build noogle yourself though. This will Take a while If you do it the First time.
You can then Override the nixpkgs-master Input with your branch.

nix build GitHub:nix-community/noogle#ui --override-input nixpkgs-master {your url}

@motiejus
Copy link
Contributor

motiejus commented Aug 5, 2024

This is perfect!

nix build GitHub:nix-community/noogle#ui --override-input nixpkgs-master {your url}

Any flags I am missing?

[motiejus@vno1-gdrx:~/code/nixpkgs]$ nix build GitHub:nix-community/noogle#ui --override-input nixpkgs-master https://github.com/NixOS/nixpkgs
error: 'GitHub:nix-community/noogle#ui' is not a valid URL

[motiejus@vno1-gdrx:~/code/nixpkgs]$ 

@hsjobeki hsjobeki mentioned this pull request Aug 5, 2024
10 tasks
@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 5, 2024

Github needs to be lowercase. Got autocorrected in my Phone 😅

nix build github:nix-community/noogle#ui --override-input nixpkgs-master github:NixOS/nixpkgs/master

This should do (you can also replace master with your branch and NixOS with your fork)

@philiptaron
Copy link
Contributor

I'm getting

error: NAR hash mismatch in input 'https://registry.npmjs.org/@corex/deepmerge/-/deepmerge-4.0.43.tgz?narHash=sha256-GlUW2/0Z1wRF1RyrT/La9POas%2Bbv9x%2BTbbCQy9V3bW8%3D', expected 'sha256-GlUW2/0Z1wRF1RyrT/La9POas+bv9x+TbbCQy9V3bW8=' but got 'sha256-Cr1OYmR1+ozY0Hwny+oQ3BHvioS5+suPrMOujPio8+w='

Problem on master?!

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 5, 2024

Problem on master?!

Uh, shouldn't it was build from github ci 8 hrs ago.

Are you building on darwin?

Otherwise it seems some random npm dependency has changed the nar hash / integrity... which maybe ci doesnt experience, since it has nix-community binary cache ...

I'll update the npm lockfile real quick

@hsjobeki
Copy link
Contributor Author

hsjobeki commented Aug 5, 2024

Yep. Seems npm registry behavior changed. It'll be fixed anytime (minutes). We learn that registry sucks, and sometimes dependencies randomly change their hash ....

@motiejus
Copy link
Contributor

motiejus commented Aug 5, 2024

This seems to build for this PR:

nix build github:nix-community/noogle#ui --override-input nixpkgs-master github:hsjobeki/nixpkgs/doc/compressDrv

Both http://localhost:8000/f/pkgs/compressDrvWeb and http://localhost:8000/f/pkgs/compressDrv look great. Thanks!

:::
*/
drv:
{ formats, compressors, ... }:
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps

Suggested change
{ formats, compressors, ... }:
{ formats, compressors }:

while at it?

Copy link
Contributor

Choose a reason for hiding this comment

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

Scratch that, there is one more place that needs, I will make a PR myself.

Copy link
Contributor

Choose a reason for hiding this comment

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

@philiptaron philiptaron merged commit 1e6e7fa into NixOS:master Aug 5, 2024
@hsjobeki hsjobeki deleted the doc/compressDrv branch August 6, 2024 21:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 0 This PR does not cause any packages to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants