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.

Sign up for GitHub

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

Jump to bottom

writeShellApplication: allow substitutions and remote building. #222136

Merged
merged 1 commit into from
Apr 3, 2023
Merged

writeShellApplication: allow substitutions and remote building. #222136

merged 1 commit into from
Apr 3, 2023

Conversation

andrewhamon
Copy link
Contributor

@andrewhamon andrewhamon commented Mar 20, 2023 •
edited
Loading

Description of changes

Unlike most other trivial builders, writeShellApplication has a relatively heavy checkphase that invokes shellcheck. On my system (m1 macbook pro), a small writeShellApplication takes over 1 second to build. It also causes shellcheck to be downloaded (or in some cases shellcheck must be built, on an a fresh and unlucky unstable commit) which also adds to the overhead (my measurements were with shellcheck already downloaded).

This PR allows substitutions and stops preferring local builds for writeShellApplication.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.05 Release Notes (or backporting 22.11 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.

@RaitoBezarius
Copy link
Member

Thank you for your contribution, please read the CONTRIBUTING guide to change the target of this PR to staging without mass pinging everyone. In the meantime, I will convert it to draft.

@RaitoBezarius RaitoBezarius marked this pull request as draft March 20, 2023 13:37
@andrewhamon andrewhamon changed the base branch from master to staging March 20, 2023 15:21
@andrewhamon andrewhamon force-pushed the allow-substitutions-for-write-shell-application branch from 4acccdd to 2273732 Compare March 20, 2023 15:21
@github-actions github-actions bot added 6.topic: erlang 6.topic: haskell 6.topic: kernel The Linux kernel 6.topic: nim Nim programing language 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: ocaml 6.topic: policy discussion 6.topic: python 6.topic: qt/kde 6.topic: rust 6.topic: vim 6.topic: vscode 8.has: changelog 8.has: documentation This PR adds or changes documentation 8.has: module (update) This PR changes an existing module in `nixos/` labels Mar 20, 2023
@andrewhamon andrewhamon force-pushed the allow-substitutions-for-write-shell-application branch from 2273732 to 013a3dc Compare March 20, 2023 15:28
@github-actions github-actions bot removed the 8.has: module (update) This PR changes an existing module in `nixos/` label Mar 20, 2023
@andrewhamon
Copy link
Contributor Author

please read the CONTRIBUTING guide to change the target of this PR to staging without mass pinging everyone. In the meantime, I will convert it to draft.

I did these in the wrong order and I now understand why you instructed me in that exact order 😅 Sorry to anyone who was pinged (maybe the draft status helped prevent notifications?).

Anyways, I think I have everything fixed up, converting back to a regular PR.

@andrewhamon andrewhamon marked this pull request as ready for review March 20, 2023 15:34
@lovesegfault lovesegfault self-requested a review March 28, 2023 11:38
@lovesegfault
Copy link
Member

@andrewhamon This doesn't actually need to target staging, ofborg suggested so b/c when you forgot the inherit you changed everything that used writeText.

Please re-target to master, being careful not to create a mega-ping 😄

@andrewhamon andrewhamon marked this pull request as draft April 2, 2023 21:31
@andrewhamon andrewhamon changed the base branch from staging to master April 2, 2023 21:33
@github-actions github-actions bot added 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: rust 6.topic: vim 8.has: module (update) This PR changes an existing module in `nixos/` labels Apr 2, 2023
@andrewhamon
writeShellApplication: allow substitutions and remote building.
e929a99 
Unlike most other trivial builders, writeShellApplication has a
relatively heavy checkphase that invokes shellcheck. On my system,
a small writeShellApplication takes over 1 second to build.

This PR allows substitutions and stops preferring local builds for
writeShellApplication.
@andrewhamon andrewhamon force-pushed the allow-substitutions-for-write-shell-application branch from 013a3dc to e929a99 Compare April 2, 2023 21:36
@github-actions github-actions bot removed 6.topic: vim 8.has: module (update) This PR changes an existing module in `nixos/` 6.topic: nixos Issues or PRs affecting NixOS modules, or package usability issues specific to NixOS 6.topic: python 6.topic: rust labels Apr 2, 2023
@andrewhamon andrewhamon marked this pull request as ready for review April 2, 2023 21:37
@andrewhamon
Copy link
Contributor Author

Okay, mistakes were made again. I figured by now everything in staging would have worked its way to master. It wasn't quite a mega ping, but 25 commits worth. Lesson double-learned. (I did change to draft first, in case that reduces the notifications).

@lovesegfault lovesegfault merged commit e835abc into NixOS:master Apr 3, 2023
@RaitoBezarius
Copy link
Member

Okay, mistakes were made again. I figured by now everything in staging would have worked its way to master. It wasn't quite a mega ping, but 25 commits worth. Lesson double-learned. (I did change to draft first, in case that reduces the notifications).

It does reduce the notifications (for code owners) until our bot decides to mass ping everyone (even in draft mode) after ~5 minutes.

But thank you for trying :) !

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@RaitoBezarius RaitoBezarius RaitoBezarius left review comments

@lovesegfault lovesegfault Awaiting requested review from lovesegfault

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@andrewhamon @RaitoBezarius @lovesegfault