Skip to content

duckstation: 0.1-6292 -> 0.1-6658 #309064

Merged
eclairevoyant merged 3 commits intoNixOS:masterfrom
atorres1985-contrib:duckstation
May 29, 2024
Merged

duckstation: 0.1-6292 -> 0.1-6658 #309064
eclairevoyant merged 3 commits intoNixOS:masterfrom
atorres1985-contrib:duckstation

Conversation

@AndersonTorres
Copy link
Member

@AndersonTorres AndersonTorres commented May 4, 2024 •
edited
Loading

Description of changes

Closes #308974 (by assimilation)

Things done

  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandboxing enabled in nix.conf? (See Nix manual)
    • sandbox = relaxed
    • sandbox = true
  • Tested, as applicable:
  • Tested compilation of all packages that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD". Note: all changes have to be committed, also see nixpkgs-review usage
  • Tested basic functionality of all binary files (usually in ./result/bin/)
  • 24.05 Release Notes (or backporting 23.05 and 23.11 Release notes)
    • (Package updates) Added a release notes entry if the change is major or breaking
    • (Module updates) Added a release notes entry if the change is significant
    • (Module addition) Added a release notes entry if adding a new NixOS module
  • Fits CONTRIBUTING.md.

Add a 👍 reaction to pull requests you find important.

@AndersonTorres AndersonTorres marked this pull request as ready for review May 4, 2024 16:20
@nixos-discourse
Copy link

nixos-discourse commented May 4, 2024

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/3878

@qubitnano
Copy link
Contributor

qubitnano commented May 4, 2024

Result of nixpkgs-review pr 309064 run on x86_64-linux 1

1 package built:
  • duckstation

Thank you

@ofborg ofborg bot requested a review from guibou May 4, 2024 16:49
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. labels May 4, 2024
marcusramberg
marcusramberg approved these changes May 4, 2024
View reviewed changes
Copy link
Contributor

@marcusramberg marcusramberg left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm.

@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one person. label May 4, 2024
@getchoo
Copy link
Member

getchoo commented May 5, 2024

Result of nixpkgs-review pr 309064 run on aarch64-linux 1

1 package built:
  • duckstation

@getchoo getchoo added 12.approvals: 2 This PR was reviewed and approved by two persons. and removed 12.approvals: 1 This PR was reviewed and approved by one person. labels May 5, 2024
AndersonTorres
AndersonTorres commented May 5, 2024
View reviewed changes
@nixos-discourse
Copy link

nixos-discourse commented May 5, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/prs-already-reviewed/2617/1618

@wegank wegank removed the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 5, 2024
SuperSamus
SuperSamus reviewed May 6, 2024
View reviewed changes
SuperSandro2000
SuperSandro2000 requested changes May 6, 2024
View reviewed changes
@SuperSamus SuperSamus mentioned this pull request May 7, 2024
Merged
13 tasks
@nixos-discourse
Copy link

nixos-discourse commented May 9, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/call-for-discussion-why-and-how-much-should-we-care-about-the-override-interfaces-part-i-duckstation-and-pcsx2/45117/1

doronbehar
doronbehar reviewed May 9, 2024
View reviewed changes
doronbehar
doronbehar reviewed May 15, 2024
View reviewed changes
doronbehar
doronbehar reviewed May 16, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks much better now!

@SuperSamus SuperSamus mentioned this pull request May 20, 2024
Closed
13 tasks
AndersonTorres
AndersonTorres commented May 22, 2024
View reviewed changes
Copy link
Member Author

@AndersonTorres AndersonTorres left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

.

doronbehar
doronbehar requested changes May 22, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please minimize your diffs.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 22, 2024

Who added that "Check that Nix files are formatted" without telling it to everyone???

@github-actions github-actions bot added 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` and removed 8.has: maintainer-list (update) This PR changes `maintainers/maintainer-list.nix` labels May 22, 2024
@doronbehar
Copy link
Contributor

doronbehar commented May 23, 2024

CI error:

/nix/store/xfhkjnpqjwlf6hlk1ysmq3aaq80f3bjj-stdenv-linux/setup: line 1581: out/share/duckstation/common-tests: No such file or directory

@doronbehar
Copy link
Contributor

doronbehar commented May 23, 2024

Who added that "Check that Nix files are formatted" without telling it to everyone???

Where?

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 24, 2024

Now it should work.

doronbehar
doronbehar requested changes May 24, 2024
View reviewed changes
Copy link
Contributor

@doronbehar doronbehar left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm beginning to lose patience due to your insistence on enlarging the diff for no real good reason. I wanted to spare us the review-change ping pong, by rebasing your branch myself, but you are not allowing maintainers to edit the PR .

Your next trial is your last.

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 24, 2024

Anything else?

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 26, 2024

but you are not allowing maintainers to edit the PR .

I am on that web interface of GitHub, and the "allow edits by maintainers" checkbox does not exist.
I don't know why.

I have opened another PR right now and can confirm: there is no checkbox.

edits

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 26, 2024

Oh, anything else? [2]

@eclairevoyant
Copy link
Contributor

eclairevoyant commented May 26, 2024

the "allow edits by maintainers" checkbox does not exist.

https://github.com/orgs/community/discussions/5634

Currently, if a PR is created from a fork living under an organisation (rather than an individual) it's not possible to allow edits by maintainers.

SuperSandro2000
SuperSandro2000 previously requested changes May 28, 2024
View reviewed changes
@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 28, 2024

@SuperSandro2000 and @doronbehar

Your both reviews are mutually incompatible.

Bringing back qt6.callPackage is not compatible to get rid of all-packages.nix.

I am unable to satisfy both of your reviews at the same time.

Please solve this between yourselves before I can take any action (besides rebasing this over Master Branch).

@AndersonTorres
Copy link
Member Author

AndersonTorres commented May 28, 2024

And just for venting a decade-old frustration,
I have less than love for that less than beautiful dark-magic-accepted-by-blind-fanaticism thing called "splicing".

@AndersonTorres
duckstation: migrate to by-name
d9a0966 
Since by-name hierarchy does not support "third party" callPackage calls, the
expression was modified to get rid of qt6Packages.
@AndersonTorres
duckstation: checkPhase -> installCheckPhase
82a6593
@AndersonTorres
duckstation: 0.1-6292 -> 0.1-6658
9b1c668 
Duckstation now uses a vendored shaderc.
However, this vendoring is provided by a patch.
@infinisil
Copy link
Member

infinisil commented May 28, 2024

For easily finding it, a discussion is happening in this thread.

Bringing back qt6.callPackage is not compatible to get rid of all-packages.nix.

#309064 (comment) :)

Who added that "Check that Nix files are formatted" without telling it to everyone???

It's only for a small subset of files for now, people are very eager to use the standard formatter already :). For reference, #299578 introduced it, also #313628 is relevant. Also see #322520.

eclairevoyant
eclairevoyant approved these changes May 29, 2024
View reviewed changes
Copy link
Contributor

@eclairevoyant eclairevoyant left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I didn't test properly running this in a game, but builds and starts fine.

@eclairevoyant eclairevoyant dismissed SuperSandro2000’s stale review May 29, 2024 00:15

splicing works without qt6.callPackage

@wegank wegank added the 12.approvals: 2 This PR was reviewed and approved by two persons. label May 29, 2024
@eclairevoyant eclairevoyant merged commit f5e912c into NixOS:master May 29, 2024
@AndersonTorres AndersonTorres mentioned this pull request Jun 13, 2024
Merged
13 tasks
@nixos-discourse
Copy link

nixos-discourse commented Jul 25, 2024

This pull request has been mentioned on NixOS Discourse. There might be relevant details there:

https://discourse.nixos.org/t/frustrations-about-splicing/49607/1

@doronbehar doronbehar mentioned this pull request Aug 4, 2024
Closed
@AndersonTorres AndersonTorres mentioned this pull request Aug 12, 2024
Merged
13 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@marcusramberg marcusramberg marcusramberg approved these changes

@K900 K900 K900 left review comments

@doronbehar doronbehar doronbehar approved these changes

@infinisil infinisil infinisil left review comments

@getchoo getchoo getchoo approved these changes

@SuperSandro2000 SuperSandro2000 SuperSandro2000 left review comments

@guibou guibou Awaiting requested review from guibou

@SuperSamus SuperSamus Awaiting requested review from SuperSamus

+1 more reviewer

@eclairevoyant eclairevoyant eclairevoyant approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin. 10.rebuild-linux: 1-10 This PR causes between 1 and 10 packages to rebuild on Linux. 10.rebuild-linux: 1 This PR causes 1 package to rebuild on Linux. 11.by: package-maintainer This PR was created by a maintainer of all the package it changes. 12.approvals: 2 This PR was reviewed and approved by two persons.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.