plugins/lsp/pylsp: propagatedBuildInputs -> dependencies
#1893
Conversation
There was a problem hiding this comment.
LGTM, thanks!
Does this fix anything or is it just updating to the new code style?
@GaetanLepage if you have time, can you also review?
|
The build fails with:
I guess we're using an old nixpkgs lock that doesn't have the new style on this package? Maybe we should check if the |
|
I think @GaetanLepage said he'd deal with this himself using the update action |
|
This feels like another thing that'll break anyone who's nixpkgs lock is out of sync with outs... |
|
Yeah, I actually created this PR because of the error described in #1894. In nixpkgs' master every plugin uses |
|
I'm wondering if we should have a more graceful approach than handles either the old/new attr depending on which is actually present, at least for a short transition period? |
|
Oh, yeah. I also just noticed that this rename probably won't be backported to 24.05 |
That's not an issue, since our |
There was a problem hiding this comment.
LGTM! Thanks!
I'll let @GaetanLepage have the final say since he's dealing with the lockfile update and the relevant upstream changes.
| # NOTE: Depending on the exact nixpkgs rev, we may have `dependencies` or `propagatedBuildInputs` | ||
| # See https://github.com/NixOS/nixpkgs/pull/327630#discussion_r1679253679 | ||
| # TODO: Only filter `dependencies` | ||
| (lib.filter (n: old ? ${n}) [ | ||
| "propagatedBuildInputs" | ||
| "dependencies" | ||
| ]) | ||
| (n: filterDependencies old.${n}) |
There was a problem hiding this comment.
Since this is done gracefully I don't see why we can't merge now?
There was a problem hiding this comment.
If you are talking about merging the PR, tests were failing. I just merged main to here and tests are green again
There was a problem hiding this comment.
Our CI is currently disabled, so the only "test" running on GitHub is mergify (which doesn't actually test any code).
Or are you referring to tests run locally?
There was a problem hiding this comment.
I referred to this test failure in 6066ff2, and local tests fail for me even on main with some huge and totally unrelated error.
There was a problem hiding this comment.
I referred to this test failure in 6066ff2,
The linked failure is #1878 (one of the reasons CI is currently disabled).
local tests fail for me even on main with some huge and totally unrelated error.
Strange. I've just run the test suite locally on other PRs and everything passes just fine...
Are you using the tests devshell command or just nix flake check? Are you trying to use --all-systems?
There was a problem hiding this comment.
Just ran tests on this branch, and everything passes for me (x86_64-linux).
There was a problem hiding this comment.
I used nix flake check --all-systems -v and got these logs:
logs.txt.gz
(too big for pastebin, so sending as a file)
Anyway, this doesn't really matter. If all tests are passing, why not merge this PR?
There was a problem hiding this comment.
why not merge this PR?
As mentioned above, I'm delegating to @GaetanLepage here since he's dealing with these changes in nixpkgs.
|
Thank you very much @PerchunPak for spinning up this fix so quickly. Please, feel free to report any eventual weird behavior. |
See NixOS/nixpkgs#327630 (comment)
The only (patched) pylsp plugin that doesn't use
dependenciesyet ispylsp-rope, though it will change soon with NixOS/nixpkgs#328410 (pr-tracker).