Skip to content
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

beatsabermodmanager: 0.0.5 -> 0.0.7 #339370

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

ImUrX
Copy link
Contributor

@ImUrX ImUrX commented Sep 4, 2024

Description of changes

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.11 Release Notes (or backporting 23.11 and 24.05 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.

@ofborg ofborg bot requested a review from Atemu September 4, 2024 01:58
@ofborg ofborg bot added 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 labels Sep 4, 2024
Copy link
Member

@FliegendeWurst FliegendeWurst left a comment

Choose a reason for hiding this comment

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

Build fails:

building '/nix/store/h27blkagnak2sabmhipwx7kxn5jrlf9y-beatsabermodmanager-0.0.6.drv'...
Running phase: unpackPhase
unpacking source archive /nix/store/snvc6h0lg7vf7ks0d66zgfpnjf1sknx6-source
source root is source
Running phase: patchPhase
Running phase: configureNuget
error: File '/build/source/nuget.config' does not exist.

@Scrumplex
Copy link
Member

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 339370


x86_64-linux

❌ 1 package failed to build:
  • beatsabermodmanager

@Scrumplex
Copy link
Member

Please rebase on master

@NovaViper
Copy link
Contributor

Build fails:

building '/nix/store/h27blkagnak2sabmhipwx7kxn5jrlf9y-beatsabermodmanager-0.0.6.drv'...
Running phase: unpackPhase
unpacking source archive /nix/store/snvc6h0lg7vf7ks0d66zgfpnjf1sknx6-source
source root is source
Running phase: patchPhase
Running phase: configureNuget
error: File '/build/source/nuget.config' does not exist.

Same here, just tried it by overlaying it into my flake and got the same result

beatsabermodmanager> building '/nix/store/jhnhvdqxf6128lwyvpa0a6c82sm5mamc-beatsabermodmanager-0.0.6.drv'
beatsabermodmanager> Running phase: unpackPhase
beatsabermodmanager> unpacking source archive /nix/store/snvc6h0lg7vf7ks0d66zgfpnjf1sknx6-source
beatsabermodmanager> source root is source
beatsabermodmanager> Running phase: patchPhase
beatsabermodmanager> Running phase: configureNuget
beatsabermodmanager> error: File '/build/source/nuget.config' does not exist.

@FliegendeWurst FliegendeWurst added the awaiting_changes (old Marvin label, do not use) label Nov 11, 2024
@NovaViper
Copy link
Contributor

NovaViper commented Nov 12, 2024

@ImUrX Hey I noticed the source files for the tagged version of the package hasn't been updated but the prepacked binaries have (see screenshot below)... I think it may be for the best to target against the git source instead of the tagged releases since there's been a couple of commits made that fixed the application version's number. Additionally, there's actually a pretty major bug right now that I found that the manager isn't saving the mods 😓

image

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 12, 2024

😓 yeah i can update the whole thing, will do it today if i dont forget

@NovaViper
Copy link
Contributor

Additionally, there's actually a pretty major bug right now that I found that the manager isn't saving the mods

😓 Speaking of that... it also seems to be present in the latest git revision too. Just tried to build against it and was able to replicate the bug in that version too. We're gonna have to wait til that one gets ironed out unfortunately

@Atemu
Copy link
Member

Atemu commented Nov 12, 2024

cc @affederaffe

@ImUrX ImUrX changed the title beatsabermodmanager: 0.0.5 -> 0.0.6 beatsabermodmanager: 0.0.5 -> 0.0.7 Nov 13, 2024
@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 13, 2024

and sorry for the format changes, i have alexandra on save on format but i can revert those changes if needed 😓

@Atemu
Copy link
Member

Atemu commented Nov 13, 2024

If anything, use nixfmt-rfc-style and do it in a separate commit.

We have a standard on this.

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 13, 2024

so it should be another commit? but isnt that also against contribution guidelines
or can i just squash the nixfmt?

@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 13, 2024

and if it needs to be another commit, whats the naming convention? sorry for so many questions, i just read the guidelines and it just mentions

(pkg-name): (from -> to | init at version | refactor | etc)

(Motivation for change. Link to release notes. Additional information.)

@Atemu
Copy link
Member

Atemu commented Nov 13, 2024

so it should be another commit? but isnt that also against contribution guidelines

The action of formatting the file is one logical step and should therefore be one commit separated from all the other logical steps/commits.

I don't see how that violates any contributing guidelines, it tells you to do exactly that.

and if it needs to be another commit, whats the naming convention? sorry for so many questions, i just read the guidelines and it just mentions

(pkg-name): (from -> to | init at version | refactor | etc)

(Motivation for change. Link to release notes. Additional information.)

I'm not sure what you're confused about, it tells you right there. You're touching BSMM and what you're doing is apply nixfmt or IOW formatting the nix expressions. Following the pattern given that leaves us with: beatsabermodmanager: format using nixfmt. Stating a motivation isn't required here as it should be obvious.

If the way this section of the contributor's manual is written wasn't immediately understandable to you, please open an issue about that. Most of us don't frequently have a need to read and understand the contributor's manual and will therefore not be aware of any shortcomings.
Alternatively, you could even open a PR where you change the wording such that you would have understood it better. The contributor's manual isn't set in stone and any change to it that helps onboarding future contributors is welcome.

It's absolutely fine and even encouraged to ask questions if you're unsure about something but please always be specific in your questions so that we can effectively answer them.

@ImUrX ImUrX force-pushed the beatsaberbump branch 3 times, most recently from ca33a83 to 783a860 Compare November 13, 2024 03:15
@ImUrX
Copy link
Contributor Author

ImUrX commented Nov 13, 2024

cool, well should be working fine, and ready for review. thanks for the explanation!

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Diff LGTM, I'll see to test this soon.

pkgs/by-name/be/beatsabermodmanager/package.nix Outdated Show resolved Hide resolved
@wegank wegank added the 12.approvals: 1 This PR was reviewed and approved by one reputable person label Nov 13, 2024
@ofborg ofborg bot added the ofborg-internal-error Ofborg encountered an error label Nov 13, 2024
@ofborg ofborg bot requested a review from Atemu November 13, 2024 15:26
@ImUrX
Copy link
Contributor Author

ImUrX commented Dec 4, 2024

should work now i think

Copy link
Contributor

@gepbird gepbird left a comment

Choose a reason for hiding this comment

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

Thanks, changes LGTM, tested binary on x86_64-linux.

nixpkgs-review result

Generated using nixpkgs-review.

Command: nixpkgs-review pr 339370


x86_64-linux

✅ 1 package built:
  • beatsabermodmanager

@gepbird gepbird added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed ofborg-internal-error Ofborg encountered an error awaiting_changes (old Marvin label, do not use) labels Dec 4, 2024
@NovaViper
Copy link
Contributor

NovaViper commented Dec 5, 2024

Sadly still no word from the upstream maintainers regarding the flaw I found in the app a bit 😭

And now because of the dotsdk being marked as deprecated, I can't even install the app anymore since it's not updated

@wegank wegank added 12.approvals: 2 This PR was reviewed and approved by two reputable people and removed 12.approvals: 1 This PR was reviewed and approved by one reputable person labels Dec 5, 2024
@Atemu
Copy link
Member

Atemu commented Dec 5, 2024

IME the upstream dev does respond but will take a few months to do so. This is typical for OSS maintainers and I do the same for some of my projects.

I was going to give this some actual testing but, given that it doesn't eval on master anymore and that I didn't get around to it the past few weeks, I'm fine with merging this if someone tells me it successfully installs BS mods for them on a clean BeatSaber install (just rename the game files dir and verify files in Steam).

@gepbird gepbird mentioned this pull request Dec 5, 2024
13 tasks
@wegank wegank added the 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package label Dec 5, 2024
Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

The UI launches but mod categories are gone and none of the mods I previously selected are selected anymore. They're still in the config file though.

@Atemu
Copy link
Member

Atemu commented Dec 8, 2024

It's the same on 0.0.6. I'll try 0.0.5 but with the newer dotnet.

Copy link
Member

@Atemu Atemu left a comment

Choose a reason for hiding this comment

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

Doesn't work. Saving selected mods is entirely broken.

We'll have to live with 0.0.5 depending on an insecure dotnet SDK until that's fixed.

@FliegendeWurst FliegendeWurst added the 2.status: needs-changes This PR needs changes by the author label Dec 8, 2024
@Atemu Atemu added the 2.status: wait-for-upstream Waiting for upstream fix (or their other action). label Dec 8, 2024
@Atemu
Copy link
Member

Atemu commented Dec 8, 2024

Here's what you need to permit:

    nixpkgs.config.permittedInsecurePackages = [
      "dotnet-runtime-wrapped-6.0.36"
      "dotnet-runtime-wrapped-7.0.20"
      "dotnet-runtime-7.0.20"
      "dotnet-core-combined"
      "dotnet-sdk-6.0.428"
      "dotnet-sdk-7.0.410"
      "dotnet-sdk-wrapped-6.0.428"
      "dotnet-sdk-wrapped-7.0.410"
    ];

This is pain. @corngood why is every single one of these marked as insecure separately? Surely the wrappers or the combined package don't need to be as the wrapped/contained packages are already insecure.

@corngood
Copy link
Contributor

corngood commented Dec 8, 2024

@Atemu I had assumed that was because they were transitively insecure, but I guess I misunderstood. It must be because of how we're inheriting meta. It should be fixable in that case.

@Atemu
Copy link
Member

Atemu commented Dec 8, 2024

Inheriting meta whole is generally not a good idea. Always pick the parts you actually care about.

@wegank wegank added 12.approvals: 1 This PR was reviewed and approved by one reputable person and removed 12.approvals: 2 This PR was reviewed and approved by two reputable people 12.approved-by: package-maintainer This PR was reviewed and approved by a maintainer listed in the package labels Dec 8, 2024
@wegank wegank added the 2.status: merge conflict This PR has merge conflicts with the target branch label Dec 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
2.status: merge conflict This PR has merge conflicts with the target branch 2.status: needs-changes This PR needs changes by the author 2.status: wait-for-upstream Waiting for upstream fix (or their other action). 10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 12.approvals: 1 This PR was reviewed and approved by one reputable person
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants