Skip to content

vimPlugins: don't build generated plugins on hydra#377508

Merged
GaetanLepage merged 1 commit intoNixOS:masterfrom
PerchunPak:dont-build-on-hydra
Feb 5, 2025
Merged

vimPlugins: don't build generated plugins on hydra#377508
GaetanLepage merged 1 commit intoNixOS:masterfrom
PerchunPak:dont-build-on-hydra

Conversation

@PerchunPak
Copy link
Member

@PerchunPak PerchunPak commented Jan 28, 2025

Building generated plugin is really simple and its cost to serve from hydra greatly outcomes cost of locally building.

This also allows us to merge big rebuilds, where each rebuild is fairly simple, to master without delaying next branch update by a few hours (see #368843 (comment))


I planned on doing that in a rewrite of the updater, but seeing #377507, it would be useful right now

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/)
  • 25.05 Release Notes (or backporting 24.11 and 25.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.

@github-actions github-actions bot added the 6.topic: vim Advanced text editor label Jan 28, 2025
@GaetanLepage
Copy link
Contributor

I planned on doing that in a rewrite of the updater, but seeing #377507, it would be useful right now

What is the link with #377507 ? I'm not sure to understand.

@github-actions github-actions bot added 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. labels Jan 28, 2025
@khaneliman
Copy link
Contributor

I planned on doing that in a rewrite of the updater, but seeing #377507, it would be useful right now

What is the link with #377507 ? I'm not sure to understand.

Basically vim plugins wouldn’t require staging cycles for mass changes

@PerchunPak

This comment was marked as duplicate.

@PerchunPak
Copy link
Member Author

The drawback is that nixpkgs-review wouldn't test any massive rebuilds, but we can use build-by-path.sh for that

Maybe implementing Mic92/nixpkgs-review#430 would also be a fix

@khaneliman
Copy link
Contributor

The drawback is that nixpkgs-review wouldn't test any massive rebuilds, but we can use build-by-path.sh for that

Maybe implementing Mic92/nixpkgs-review#430 would also be a fix

I’d be a fan of up streaming that functionality. I use it all the time because I don’t actually care about eval either for building package sets when nix will only build what has changed.

@GaetanLepage
Copy link
Contributor

I am in favor of this change.
What is your opinion @teto ?

@teto
Copy link
Member

teto commented Jan 28, 2025

What is your opinion @teto ?

Disabling building is something I've thought about in the past, as I found the process wasteful. It has become more of an issue now that there is more activity on the nixpkgs vim side. It would be nice to still be able to build more complex neovim plugins (the ones with dependencies, so maybe all those buildNeovimPlugin).

How is one supposed to run a nixpkgs-review if the plugins are not built either ?

This also allows us to merge big rebuilds, where each rebuild is fairly simple, to master without delaying next branch update by a few hours (see #368843 (comment))

This is IMO a weak argument. We can wait a few hours/days. I suspect each contributor here runs his own fork of nixpkgs.
I am more sympathetic to the hydra argument.

@PerchunPak
Copy link
Member Author

How is one supposed to run a nixpkgs-review if the plugins are not built either ?

Then I propose to block this pull request by Mic92/nixpkgs-review#430 which would add something like --build-namespace vimPlugins

Also, do note that <plugin>.src should not be affected, so the only thing that user should run is nvimRequireCheck.

all those buildNeovimPlugin

#376370 strips out them from generated, so they will be built by hydra

This is IMO a weak argument. We can wait a few hours/days

Sure, we can. But that's an issue for other developers, an update of vim plugins should not delay update of the entire ecosystem if it is not necessary. And we should not wait for staging cycles if we can avoid it.

@GaetanLepage
Copy link
Contributor

Then I propose to block this pull request by Mic92/nixpkgs-review#430 which would add something like --build-namespace vimPlugins

So this is something that has to be implemented in nixpkgs-review ? Let's re-open the issue then, no ?

@PerchunPak
Copy link
Member Author

Mic92/nixpkgs-review#459 got merged, so let's rebase and merge this

Building generated plugin is really simple and its cost to serve from
hydra greatly outcomes cost of locally building.

This also allows us to merge big rebuilds, where each rebuild is fairly
simple, to master without delaying next branch update by a few hours
(see NixOS#368843 (comment))
@teto
Copy link
Member

teto commented Feb 2, 2025

Mic92/nixpkgs-review#459 got merged, so let's rebase and merge

We might want to wait for the patch to reach nixpkgs' nixpkgs-review to avoid an addionnal step for maintainers.

@PerchunPak PerchunPak mentioned this pull request Feb 5, 2025
13 tasks
@GaetanLepage
Copy link
Contributor

We might want to wait for the patch to reach nixpkgs' nixpkgs-review to avoid an addionnal step for maintainers.

Done ! We should be able to merge this now.

@GaetanLepage
Copy link
Contributor

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 377508


x86_64-linux

✅ 1 package built:
  • vimPluginsUpdater

aarch64-linux

✅ 1 package built:
  • vimPluginsUpdater

x86_64-darwin

✅ 1 package built:
  • vimPluginsUpdater

aarch64-darwin

✅ 1 package built:
  • vimPluginsUpdater

@PerchunPak
Copy link
Member Author

We might want to wait for the patch to reach nixpkgs' nixpkgs-review to avoid an addionnal step for maintainers.

Done ! We should be able to merge this now.

Unless we want to wait until 3.1.0 will get to the unstable channel

For now, I use this overlay
https://github.com/PerchunPak/nixos-dotfiles/blob/33dc338bc2ece03d455a26df849a202205e756f7/overlays/nixpkgs-review-3.1.0/default.nix

@GaetanLepage GaetanLepage merged commit 66555b1 into NixOS:master Feb 5, 2025
25 of 27 checks passed
@PerchunPak PerchunPak deleted the dont-build-on-hydra branch March 4, 2025 04:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

6.topic: vim Advanced text editor 10.rebuild-darwin: 1-10 This PR causes between 1 and 10 packages to rebuild on Darwin. 10.rebuild-darwin: 1 This PR causes 1 package to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants