-
-
Notifications
You must be signed in to change notification settings - Fork 14k
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
plover.dev: 4.0.0.dev10 -> 4.0.0rc2 #303669
base: master
Are you sure you want to change the base?
plover.dev: 4.0.0.dev10 -> 4.0.0rc2 #303669
Conversation
1a0e09a
to
5a1d423
Compare
Thank you for the reminder, I'll try to set some time aside this evening |
Thank you for your contribution -- I've tested and it works. I've left a few minor comments. I'm not a core maintainer (or whatever the word is) so I cannot approve but having someone look over it should make that process faster. I think once you have had a look at the comments, the next step is to post the PR into https://discourse.nixos.org/t/prs-ready-for-review/3032/3831 |
|
||
meta = with lib; { | ||
mainProgram = "plover"; | ||
broken = stdenv.isDarwin; |
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.
Given the above you have darwin in platforms, were you able test on it? If so can this broken be removed?
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.
No I'm not able to test on darwin platforms.
For the previous packages (plover_stroke
and rtf_tokenize
) should I've only listed the platforms that I've tested ?
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.
Given these packages are inaccessible outside of plover (i.e. can't just nix build nixpkgs#plover.plover_stroke
or similar) perhaps there is no need to have darwin in the list.
I'm not that knowledgeable about all the maintenance burdens and intricacies of packaging, but I believe there are two conflicting goals: one is offer as large set of available packages as possible, to make it an attractive offering as possible, the other is to minimize broken packages.
If we don't mark a package as working when it isn't people might not have the confidence to try building and then contribute a patch saying it's working (perhaps adding themselves to the maintainer list to get notified if it is updated in the future, to test it on their systems too). Given nix is already technically challenging, perhaps we could be fine with this.
Also, hydra won't have prebuilt binaries, not a huge problem for quick to build packages though.
On the maintenance side though, if we mark a package as working when it isn't, it's going to present as a failure which will increase maintenance burden (tracking down why it's failed, getting hold of the system on which it's failed). These probably outweigh the maintenance required to approve PRs adding to the supported systems.
So I would just stick with smaller list of known supported systems here, though what I'm trying to say is, it's not a case of one is strictly better than the other here. I've had a quick look in https://nixos.org/nixpkgs/manual and in README.md/pkgs/README.md and couldn't find advice on which to prefer.
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.
As you suggested, I've removed platform.darwin
from the intermediate packages.
pname = "plover"; | ||
version = "4.0.0.dev10"; | ||
version = "4.0.0.dev12"; |
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.
dev12 is from 2022-08-08, any reason for not using the latest pre-release? The 4.0.0rc2+6.g53c416f release is using a tag "continuous" which sounds like it might be deleted/renamed, so maybe not that one, but v4.0.0rc2 should work
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.
The current pattern for plover
on nixpkgs
is plover.stable
(deprecated) and plover.dev
.
Since it's my first contribution to nixpkgs
I wanted to start with something simple and providing the v4.0.0-rc.2
would have required to add a new derivation for plover.rc
(by following the previous pattern) (but I may provide a PR for that version in the future).
Instead I've choose to provide an update for plover.dev
even tho it's an old version (2022) to take a hands-on the PR process of nixpkgs
.
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.
Ah I see, I don't think you'd need to add a new derivation plover.rc
, I think plover.dev
would be fine with a release candidate version -- I think the distinction is full/stable releases vs development releases, and a release candidate I would say is a development release (on its way to being a full release though).
The other way of looking at it is, I don't believe v4.0.0rc2 breaks anything new that v4.0.0dev10 or v4.0.0dev12 doesn't already break.
I.e. both have the following
Warning: Version 4 is a major change and the configuration file it creates is not compatible with Plover 3 or earlier. Please backup your plover.cfg
and this might be the reason for plover.stable
vs plover.dev
, but it's not a reason to have plover.dev
vs plover.rc
if this makes sense.
So upgrading to v4.0.0rc2 but keeping it under plover.dev
should be fine.
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've added a commit to update to 4.0.0-rc.2
\(^^)/
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 agree with this, I'm perfectly happy for plover.dev
to be an RC. I only added the stable
/dev
split because stable
didn't have any updates for a very long time. If we consider plover.stable
to be deprecated and we don't expect many people to be using it (the last stable release of Plover is from 2017!) then I wouldn't even mind doing away with this distinction and having the main plover
attribute refer to 4.0.0-rc.2.
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.
Thanks you for your review @KoviRobi, I'll try to make a post on discourse
pname = "plover"; | ||
version = "4.0.0.dev10"; | ||
version = "4.0.0.dev12"; |
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.
The current pattern for plover
on nixpkgs
is plover.stable
(deprecated) and plover.dev
.
Since it's my first contribution to nixpkgs
I wanted to start with something simple and providing the v4.0.0-rc.2
would have required to add a new derivation for plover.rc
(by following the previous pattern) (but I may provide a PR for that version in the future).
Instead I've choose to provide an update for plover.dev
even tho it's an old version (2022) to take a hands-on the PR process of nixpkgs
.
|
||
meta = with lib; { | ||
mainProgram = "plover"; | ||
broken = stdenv.isDarwin; |
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.
No I'm not able to test on darwin platforms.
For the previous packages (plover_stroke
and rtf_tokenize
) should I've only listed the platforms that I've tested ?
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/3839 |
73eac0a
to
83c4b43
Compare
@SuperSandro2000 thanks for the review, I've applied your suggestions:
For the |
propagatedBuildInputs = [ | ||
babel | ||
pyqt5 | ||
qt5.qtwayland |
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.
We probably want to place qtwayland only in buildInputs. It will then be used by the wrapper and we don't propagate unnecessary libraries by accident.
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've moved the qt5.qtwayland
to buildInputs
as suggested
This updates the Plover development version to 4.0.0.dev12. - I was having an issue first when build the project without `pyproject = true`. Indicating that `Requirements should be satisfied by a PEP 517 installer.` - With `pyproject` enabled, 2 dependencies were missing: `plover_stroke` and `rtf_tokenize`. - `postPatch` step was removed since the `setup.cfg` file no longer specify `PyQt5`.
c6253fe
to
186e5a6
Compare
Description of changes
Changelog for plover-v4.0.0rc2.
This updates the Plover development version to 4.0.0.rc2.
I was having an issue first when build the project without
pyproject = true
.The error was
Requirements should be satisfied by a PEP 517 installer.
With
pyproject
enabled, 2 dependencies were missing:plover_stroke
andrtf_tokenize
.postPatch
step was removed since thesetup.cfg
file no longer specifyPyQt5
.Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-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.