3 posts were merged into an existing topic: [Discussion] PR reviews
@tomberek thank you for starting with the reviews
Please do remember to do your review as a GitHub review (Files changed → Review changes). That has several advantages:
- the PR will no longer show up in the filtered view I link from the top post
- it is easier to understand the reasoning later
- you will get subscribed to the PR in case there will be future discussion
- as a little bonus there is a gamification aspect: it will be possible to generate statistics who reviewed how many PRs
This post is only to get someone to act on the review. It should be enough to just post “merge: <link>” or you could copy your review here. But it should not only be here, especially if these posts are deleted.
Regarding your second review: Please only post clear-cut reviews here. Your review is of course helpful and you could open a discussion in the PR and maybe ask for advice on IRC or a separate discourse post. But this post should be reserved for cases where the committer only has to “review a review”, not make a decision themselves or be particularly familiar with the particular packages.
Please don’t read the remarks in any negative way, I’m glad you started with the reviews This is just the first step in refining the process.
Finished~~merge: https://github.com/NixOS/nixpkgs/pull/58681~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58703~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58762~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58862~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57478~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58008~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58005~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57335~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58656~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58316~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58673~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58299~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58883~~ ~~re-run ofborg: https://github.com/NixOS/nixpkgs/pull/58909~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/49868~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52589~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/52948~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56809~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/57937~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59077 (approved by @matthewbauer ,awaiting merge, minimal impact)~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59307~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/56418~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59502~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59474~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59493~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59476~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59447~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59383~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59436~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59536~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59634~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59649~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60051~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60205~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60190~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60156~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/59643~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/58774~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60338~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60160~~ ~~needs more reviewers: https://github.com/NixOS/nixpkgs/pull/60102~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60484~~ ~~merge: https://github.com/NixOS/nixpkgs/pull/60486~~ merge: https://github.com/NixOS/nixpkgs/pull/60494 merge: https://github.com/NixOS/nixpkgs/pull/60531 merge: https://github.com/NixOS/nixpkgs/pull/60532 merge: https://github.com/NixOS/nixpkgs/pull/60547 merge: https://github.com/NixOS/nixpkgs/pull/60550 merge: https://github.com/NixOS/nixpkgs/pull/60307 merge: https://github.com/NixOS/nixpkgs/pull/60281 merge: https://github.com/NixOS/nixpkgs/pull/60315 merge: https://github.com/NixOS/nixpkgs/pull/60277
needs more testing/reviewers: gawk: 4.2.1 -> 5.0.1 by r-ryantm · Pull Request #59503 · NixOS/nixpkgs · GitHub
merge or deny: https://github.com/NixOS/nixpkgs/pull/47794
merge: nixbox: init at unstable-2019-04-20 by LinArcX · Pull Request #59967 · NixOS/nixpkgs · GitHub
fix darwin: bfs: 1.3.3 -> 1.4 by r-ryantm · Pull Request #60474 · NixOS/nixpkgs · GitHub
I’ll start easy
merge: vo-amrwbenc: Fix an incorrect license. by andrewchambers · Pull Request #58643 · NixOS/nixpkgs · GitHub
merge: ejabberd: 18.12.1 -> 19.02 by Izorkin · Pull Request #56464 · NixOS/nixpkgs · GitHub
merge: fcitx: 220.127.116.11 -> 18.104.22.168 by teto · Pull Request #58442 · NixOS/nixpkgs · GitHub
I don’t know enough about node to say this is good, but I have reviewed the module and everything there looks good. If someone would like to review the package that would be appreciated:
A post was merged into an existing topic: [Discussion] PR reviews
These are simple updates. I tested both with nix-review but since both have higher rebuild count I would like to have a second opinion:
review: hdf5: 1.10.4 -> 1.10.5 by markuskowa · Pull Request #58803 · NixOS/nixpkgs · GitHub
review: v4l-utils: 1.16.2 -> 1.16.5 by markuskowa · Pull Request #53460 · NixOS/nixpkgs · GitHub
2 posts were merged into an existing topic: [Discussion] PR reviews
merge: scribus: 1.4.7 -> 1.4.8, dep fixups by dtzWill · Pull Request #57359 · NixOS/nixpkgs · GitHub
merge: gotop: 2.0.1 -> 3.0.0 by marsam · Pull Request #58615 · NixOS/nixpkgs · GitHub
add tag “status: wait-for-upstream”: libtorrentRasterbar: 1.1.11 -> 1.2.0 by r-ryantm · Pull Request #54179 · NixOS/nixpkgs · GitHub
@NRHelmi please review the list of requirements for PRs to post here. I understand that you’re probably frustrated, especially since your first contribution has been sitting there for 3 months. You’re encouraged to
- ping someone you think might be qualified/interested to review in the PR. Infinisil has already reviewed it once and may be willing to give it another look if you remind him
- ask for a review on IRC or on discourse. Maybe start another iteration of “PRs ready for review”
Unfortunately I’m not very qualified to review nixos modules myself and there is one outstanding comment by Infinisil (aws-xray-daemon-V3.0.0 | awx-xray-daemon: init at V3.0.0 by NRHelmi · Pull Request #53565 · NixOS/nixpkgs · GitHub) who has much more experience with that than I do.
Those PRs both have merge conflicts, so are not ready to merge. They are also from nixpkgs members, so posting them here is not necessary (they can merge themselves after you’ve given a positive review).
merge - neopg: 0.0.4 -> 0.0.6 by erictapen · Pull Request #59942 · NixOS/nixpkgs · GitHub
merge - teeworlds: 0.7.2 -> 0.7.3 by dtzWill · Pull Request #59971 · NixOS/nixpkgs · GitHub
merge - steamrt: fix update script by nyanloutre · Pull Request #59899 · NixOS/nixpkgs · GitHub
merge - nginxMainline: 1.15.10 -> 1.15.12 by Izorkin · Pull Request #59950 · NixOS/nixpkgs · GitHub
merge - tmux: 2.8 -> 2.9 by xrelkd · Pull Request #60194 · NixOS/nixpkgs · GitHub
merge - ipfs: 0.4.19 -> 0.4.20 by elitak · Pull Request #60148 · NixOS/nixpkgs · GitHub
merge - Datadog: update packages by Izorkin · Pull Request #59872 · NixOS/nixpkgs · GitHub
If someone can validate or propose a comment for this pr?
It fixes a bug that prevents to use diskImageFuns.debian9x86_64 and thus generate debian9 debs for example. The workaround is to use the debian8 image for the moment.
davidak via Nix community firstname.lastname@example.org writes:
I left a review for this one:
These two have both been reviewed by someone else, but could be acted on quickly by a committer:
I’m posting here my own PR, because I got 2 positive reviews, but I think it got lost:
clojure: 22.214.171.1242 -> 126.96.36.1992 by jlesquembre · Pull Request #63942 · NixOS/nixpkgs · GitHub