-
-
Notifications
You must be signed in to change notification settings - Fork 17.6k
nushellPlugins.*: refactor, add checks, mark some as broken #420675
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
Conversation
|
I hope that, when I undraft this, I don't ping like ~20 people. I am afraid that's what has to happen if you refactor 12 packages as once. I am sorry in advance if this causes anybody any trouble. |
|
pkgs/shells/nushell/plugins/hcl.nix
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed? Upstream might add tests in the future which will then be ignored until someone stumbles upon on this line.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can test, but i seem to remember that yes it is needed.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just following https://github.com/NixOS/nixpkgs/blob/d85e3b87ffdc6e879499e54599a9d606b90955ce/doc/languages-frameworks/rust.section.md#disabling-package-tests-disabling-package-tests here:
If no tests exist -- the checkPhase should be explicitly disabled to skip unnecessary build steps to speed up the build
|
I never added the smoke test to every plugin since I couldn't decide where to put it, but I think making a local install check hook would be good. |
mgttlinger
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good to me
happysalada
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I dont think the changes here are very controversial, im thinking of merging this in 24h. Happy to delay, if anyone needs time, just post a message.
|
Oh minor thing actually, there are 59 commits, could you squash then into one commit per package ? I dont think we need that granularity for those changes. |
|
I thought of doing that. But I was hoping this granularity would allow for an easier review process (before and after this merge) Also, each |
f3a9802 to
e067c17
Compare
|
I've kept the old version of the branch around, so I can revert it back if anyone objects to the suggestion by @happysalada |
This comment was marked as resolved.
This comment was marked as resolved.
6809958 to
9d1069b
Compare
|
Turns out some of them are broken because they are outdated. Marking them as such. Only found that out because I added the smoke test to all of them. Turns out that's already paying off 😅 |
Because otherwise: - The names are inconsistent with the other nushell plugins - `versionCheckHook` cannot find the binaries without setting a special variable
Why for each all at the same time? Because all nushell plugins should respond in the same way with either no arguments passed or with `--help`, which contains their version. Also because when new plugins get added or plugins get updated, problems can be easily spotted because the errors are really loud.
9d1069b to
1ccdc4d
Compare
|
Also, should this get backported to stable (except for marking |
1ccdc4d to
3746764
Compare
|
While rebasing #420202 and #420876 I noticed that the check version test fails for plugins with version Sure, patching the version in Cargo.toml is an option. substituteInPlace Cargo.toml \
--replace-fail 'version = "${lib.head (lib.splitString "-" finalAttrs.version)}"' 'version = "${finalAttrs.version}"'But I don't like patching only to pass that check. It feels like this pr introduced a "policy" to This will lead to problems when trying to update plugins after a nushell-protocol update. I suggest using a version test that allows this. |
|
You can set |
|
Maybe like this #421842 yes.
|
|
I don't see why, because in theory these plugins can get updated seamlessly by r-ryantm and maintainers wouldn't need to do a lot about it except test and merge the bot PR. Currently I don't see any plugin that has |
I thought about plugins where we have something like the following on a regular basis. |
This PR modernizes the 12 existing nushell plugins in the repo. I noticed a lot of copy-pasting has been done, with a lot of the same problems in it, so I did all of it in one go.
The new packages should be functionally equivalent, because all I did was reformat/rearrange things.
Summary:
finalAttrsand removewith lib;useCargoFetchVendorbuildAndtestSubdir,checkFlagsetc. instead of a manualcheckPhasenushell_plugin_*->nu_plugin_*to reflect the main binary name (forversionCheckHookand consistency because there were already files withnu_plugin_*as pname)meta.platforms.allpnameversionCheckHookto all of themI did all of it manually, which is error-prone but I didn't think it was worth automating.
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.