nushellPlugins.*: replace version checks #421842
Conversation
nixpkgs-ci
bot
added
10.rebuild-linux: 1-10
|
dtomvan
left a comment
dtomvan
left a comment
There was a problem hiding this comment.
Diff LGTM. Something to think about though, is that this way the version check is an optional one. Contributors need to remember to run the tests manually (this was true before also with loadCheck but that one cannot easily get done as part of the build I don't think).
Again, sorry for breaking your PRs, I was just working off of what was already there in the tree, and those all used the same "we bump our version every time there's a new nushell" pattern, so I couldn't predict that this was going to happen.
I would still prefer if you set dontVersionCheck = true; on any packages that might fail in your open PRs and defined a version check like this as an exception, not the rule.
But I defer to other reviewers, because the ikea effect might be at play here 😅
Maybe a alternative hook? If we want we could run both tests via postInstallCheck right?
No worries.
For current nutshellPluigins yes, for other existing nutshell pluigins maybe and for nixpkgs as a whole
I would also really like to hear others options. I could also imagine adding a allowUnstableSuffix option to versionCheckHook would be a option. |
timon-schelling
force-pushed
the
nushellPlugins-allow-unstable-version-names
branch
from
738581e to
4594b7c
Compare
|
I don't like how dbus is passed to the dbus plugin pkg. |
With `versionCheckHook` plugins are not allowed to have a `-unstable` suffix unless the executable also reports it in that way. `versionCheck` allows plugin version to have a `-unstable` suffix without expecting the plugin to report it that way.
|
I agree, but it's so that the plugin can access the actual dbus, instead of itself. Because a new scope has been created, if it were to import |
timon-schelling
force-pushed
the
nushellPlugins-allow-unstable-version-names
branch
from
4594b7c to
f5ce168
Compare
yeah I know, but still doesn't feel right |
|
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-ready-for-review/3032/5728 |
3 participants
see #420675 (comment)
With
versionCheckHookplugins are not allowed to have a-unstablesuffix unless the executable also reports it in that way.versionCheckallows plugin version to have a-unstablesuffix without expecting the plugin to report it that way.Things done
nix.conf? (See Nix manual)sandbox = relaxedsandbox = truenix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/)Add a 👍 reaction to pull requests you find important.