-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
pcsx2: 1.7.5779 -> 1.7.5919 #324113
pcsx2: 1.7.5779 -> 1.7.5919 #324113
Conversation
Result of 1 package built:
|
Tested through to a game -- seemed to launch and play fine. |
@getchoo thanks for the feedback, ready for re-review 🙏🏻 |
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.
Diff LGTM. Two non-blocking nits.
src = fetchFromGitHub { | ||
owner = "google"; | ||
repo = "shaderc"; | ||
rev = "v${version}"; |
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.
rev = "v${version}"; | |
rev = "refs/tags/v${version}"; |
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.
Is this causing bugs?
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, but "v${version}"
could refer to a branch if one were created with that name.
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.
Upstream could also just force‐push a different tag pointer if they wanted to break things, right? Not objecting to the principle, just wondering if this has ever come up in practice.
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.
My concern is more about the ambiguity than the possibility of breaking stuff. If they wanted to guarantee a breakage, they could delete the repo.
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 remember this happening a long time ago with nickel.
But I think we should not be overzealous with this.
@@ -55,23 +54,12 @@ llvmPackages_17.stdenv.mkDerivation (finalAttrs: { | |||
src = fetchFromGitHub { | |||
owner = "PCSX2"; | |||
repo = "pcsx2"; | |||
fetchSubmodules = true; | |||
rev = "v${finalAttrs.version}"; |
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.
rev = "v${finalAttrs.version}"; | |
rev = "refs/tags/v${finalAttrs.version}"; |
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.
What is the meaningful difference of adding refs/tags/
here? (Since v${version}
works.)
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.
That refs/tags/ is not ambiguous, just using v${version} could also mean a branch.
This pull request has been mentioned on NixOS Discourse. There might be relevant details there: https://discourse.nixos.org/t/prs-already-reviewed/2617/1805 |
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.
LGeTM
Description of changes
This includes work from @SuperSamus 's original PR (#322142) with the addition of
x86_64-darwin
hash.Changes propagated from the previous PR:
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.