-
-
Notifications
You must be signed in to change notification settings - Fork 16.6k
python3Packages.diagrams: fix add missing pngs #417585
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
base: master
Are you sure you want to change the base?
Conversation
@@ -34,6 +34,12 @@ buildPythonPackage rec { | |||
url = "https://github.com/mingrammer/diagrams/commit/59b84698b142f5a0998ee9e395df717a1b77e9b2.patch"; | |||
hash = "sha256-/zV5X4qgHJs+KO9gHyu6LqQ3hB8Zx+BzOFo7K1vQK78="; | |||
}) | |||
# Fix poetry include, https://github.com/mingrammer/diagrams/pull/1128 | |||
(fetchpatch { |
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.
Please vendor this patch.
https://github.com/NixOS/nixpkgs/tree/master/pkgs#patches
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.
In the following cases, a .patch file should be added to Nixpkgs repository, instead of retrieved:
solves problems unique to packaging in Nixpkgs
cannot be fetched easily
has a high chance to disappear in the future due to unstable or unreliable URLs
None of these apply to this situation.
- The problem is not unique to Nixpkgs. The brokenness happens for the Wheel output from recent versions of poetry.
- It is easy to fetch from the commit upstream, and another such patch is already in this recipe.
- The patch URL is not going to change.
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.
When adding patches in this manner you should be reasonably sure that the used URL is stable. Patches referencing open pull requests will change when the PR is updated and code forges (such as GitHub) usually garbage collect commits that are no longer reachable due to rebases/amends.
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.
Could you help me understand your concern?
Patches referencing open pull requests will change when the PR is updated
Who do you imagine is going to update mingrammer/diagrams#1128?
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.
Could you help me understand your concern?
Have you noticed that this paragraph is just above the lines you referenced?
Could you help me understand your concern?
Patches referencing open pull requests will change when the PR is updated
Who do you imagine is going to update mingrammer/diagrams#1128?
Could you read the sentence after that?
and code forges (such as GitHub) usually garbage collect commits that are no longer reachable due to rebases/amends.
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.
Could you make sure that
- The author will never ask you to modify the change with squashing/force pushing
- The author will not push to your branch
- The author will not squash-merge your PR
?
Even if the author assures you that they won't do that, it's part of the process to avoid such things from happening, and we have no responsibility to do your case study.
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.
Okay, assuming I duplicate my patch to be vendored in this commit, what about the other instance of this immediately above the patch I was adding?
Do I need to open a separate PR to vendor that patch too or can it be left alone to possibly break later?
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 vendor that patch as well.
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.
Okay, I've vendored both of those patches.
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.
Thank you for spelling out the reasoning behind the policy.
faa530e
to
f9994f4
Compare
Pull in resource folder when poetry builds the wheel Fixes NixOS#406527 Vendor 'Add build-system section' patch per vendoring policy
f9994f4
to
7b2323b
Compare
Pull in the
resource
folder when poetry builds the wheel.This change uses the patch from mingrammer/diagrams#1128
Fixes #406527
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.