-
-
Notifications
You must be signed in to change notification settings - Fork 14.3k
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
libpqxx: 7.7.5 -> 7.9.2 #345941
libpqxx: 7.7.5 -> 7.9.2 #345941
Conversation
bed0888
to
16a84d0
Compare
2a1e028
to
fa502ae
Compare
fa502ae
to
5e3e504
Compare
Did you rebase on last master? Some of these packages have been recently fixed (last week or the week before). |
5e3e504
to
b40ca7f
Compare
That error seems to be related to the SCons version: |
2c64995
to
f722da5
Compare
I'll work on adding that patch for |
f722da5
to
80167e6
Compare
80167e6
to
180897d
Compare
180897d
to
c526fea
Compare
|
would be great to get this merged -- it fixes some build errors with clang-19 @NixOS/postgres |
AFAIK there is nothing else to be done here. It's ready for review/merge! Feel free to review/merge. |
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.
LGTM. can remove CXXFLAGS
as code compiles with clang fine without it.
|
Release: https://github.com/jtv/libpqxx/releases/tag/7.9.2 - split development output - enable strictDeps - exclude postgres server files (uses lib/dev output instead) - migrate sha256 to hash - meta: add downloadPage, changelog
c526fea
to
da563aa
Compare
failures are broken on master and have been for a month or more. |
|
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.
It's really hard to see which changes are just "reformatting / refactor / modernize" and which changes are actually fixing something, or related to the update, or..
Could you split those commits up better, so that it's actually reviewable? I'd like to understand which change needed to happen because of the update for example, and whether there where any changes which came on top to fix stuff.
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.
Other than this nit, LGTM
As previously mentioned, can you please split up your commits? At least put the refactor and version bump changes in separate commits. |
nitpicked to death. thanks for trying to update the package, would've been nice for users. |
to reviewers: I get how constant nixfmt can be annoying but in this change what we were dealing with was like 4 lines of excess diffs. I think the review criteria can be relaxed a little bit as this change was a net improvement for nixpkgs. |
Sorry if I was too harsh, I'll try to compose my messages in a nicer way in the future. However I still want to strive for code quality and that unfortunately includes nitpicks. You are more than welcome to ask for help when you're unsure how to make a suggested change. You can also say no to nitpicks, I understand that you may not have the time to deal with them or you think they are not important at all, but PLEASE communicate that to us. Just closing the PR then complaining about nitpicks is not the best response. |
I generally agree that nitpicking can be quite annoying. And I agree that we should merge quickly when PR are a net improvement. However, to be honest, this PR didn't get nitpicked too much. The author only got "LGTM" and minor fixes. I think this is just an issue with expectations not at the right place, because here, the number of comments and asked changes is still low, from my perception. The author didn't even explained why they closed it. Maybe they just simply don't care any more about this package. (If you want an example of a PR with only a moderate amounts of comments, see #312381 for instance (62 comments, most of them after the initial maintainer approved). I'm sure we can easily find worse examples). |
I think it's reasonable to expect the changes to be split up in a way that makes proper reviewing easy, not hard. * I also think it's entirely unreasonable to expand scope here by asking for a move to the finalAttrs pattern. ** I see much less of a problem with "nit-picking", but much more of a problem with "expanding scope". That just happens way too often. I am fine with nit-picking on even small things that might have been changed to something slightly worse than before. If you change it - you better change it in a good way. But asking to change stuff that the author didn't even touch... * There are more good reasons for splitting up commits properly than just reviewing. What if one part of your commit needs to be reverted, because it broke something? You'd want to be able to revert a commit without reverting all the formatting changes, the good refactors etc. at the same time. ** Not even considering that "move everything to finalAttrs" is still seen critical by some and hardly a consensus reached by everyone. |
libpqxx: 7.7.5 -> 7.9.2
Release: https://github.com/jtv/libpqxx/releases/tag/7.9.2
Closes #279321
Notes on downstream breakage
apacheHttpdPackages.mod_tile
: flaky build, works on retrypython3Packages.python-mapnik
: python3Packages.python-mapnik: mark broken #355713t-rex
: Build failure: t-rex #355705All failures are accounted for.
Status
This PR is ready for review/merge.