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

bespokesynth: update to latest commit #242698

Merged
merged 1 commit into from
Sep 7, 2023

Conversation

PowerUser64
Copy link
Contributor

Description of changes

The developer of bespoke synth unfortunately doesn't have much time to devote to creating version number releases for the project, so the current "stable" release has gone untouched for quite some time. Currently, the latest commit/nightly release is far more stable and featureful than the current stable release. This PR updates the BespokeSynth package to the latest commit.

Things done
  • Built on platform(s)
    • x86_64-linux
    • aarch64-linux
    • x86_64-darwin
    • aarch64-darwin
  • For non-Linux: Is sandbox = true set in nix.conf? (See Nix manual)
  • 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/)
  • 23.11 Release Notes (or backporting 23.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.

@tobiasBora
Copy link
Contributor

Nice, thanks for helping us with this! Do you think you could also take this opportunity to fix the way we patch the program, by directly using rpath instead of LD_LIBRARY_PATH as we were discussing here, in order to provide a smoother experience for devs? Should be just a matter of removing:

--prefix LD_LIBRARY_PATH : '${lib.makeLibraryPath [
          libXrandr
          libXinerama
          libXcursor
          libXScrnSaver
        ]}

and adding:

NIX_LDFLAGS = "-rpath ${lib.makeLibraryPath (with xorg; [ libX11 libXrandr libXinerama libXext libXcursor libXScrnSaver ])} $NIX_LDFLAGS";
dontPatchELF = true; # needed or nix will try to optimize the binary by removing "useless" rpath

Thanks!

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Breaks Darwin builds as-is.

Screenshot_macOS_2023-07-10_22:53:54

@PowerUser64
Copy link
Contributor Author

It seems that trying to pass through NIX_LDFLAGS in the same way we were doing in the flake causes it to fail compiling a basic test program because the variable isn't expanded. I've gone removed $NIX_LDFLAGS from the end of the NIX_LDFLAGS = ... line.

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

For fixing Darwin support, please add MetalKit to the Darwin-specific buildInputs and, in pkgs/top-level/all-packages.nix, change

  bespokesynth = callPackage ../applications/audio/bespokesynth {
    inherit (darwin.apple_sdk.frameworks) Accelerate Cocoa WebKit CoreServices CoreAudioKit IOBluetooth;
  };

to

  bespokesynth = darwin.apple_sdk_11_0.callPackage ../applications/audio/bespokesynth {
    inherit (darwin.apple_sdk_11_0.frameworks) Accelerate Cocoa WebKit CoreServices CoreAudioKit IOBluetooth MetalKit;
  };

@PowerUser64
Copy link
Contributor Author

@OPNA2608 does this do it?

@tobiasBora
Copy link
Contributor

I can confirm that it works for what I tested on NixOs! (VST version etc), good for me if it also works on MacOs!

@PowerUser64
Copy link
Contributor Author

PowerUser64 commented Jul 11, 2023

Quick question, I'm going to try to keep this up-to-date as much as possible. I don't want you all to hate me because I'm creating/updating PR's every time there's a new bespoke nightly. Should I try to limit PR's to one per week or something? How should we handle this?

For reference, bespoke nightly comes out about one per commit usually. This can as often as anywhere from one every two weeks or (especially recently), one or more per day. I always run the latest version with overrideAttrs in my system config, so I should always know whether it's broken on Linux at least.

@tobiasBora
Copy link
Contributor

tobiasBora commented Jul 11, 2023

@PowerUser64 Well, I am not sure if nixpkgs is well suited for nightly versions (but great to see that this project is getting much attention). Since the review process can be quite long and energy consuming (we need to test across several systems like MacOs to be sure there is no breakage, find a nix maintainer to merge it, wait for hydra to build it etc…) I would say that adding one release per day, or even one per month for a basic software is too much. Every 3 months is maybe more reasonable, unless a really important bug/security issue is fixed (but you might ask for confirmation to more experienced users, e.g. in matrix or discourse).

If you want to deal with nightly build, I would rather advice you create your own repository (It is what people are doing for instance emacs) and to integrate it into NUR. You might want to use something like cachix if you want others to be able to use you pre-built binaries.

@tobiasBora
Copy link
Contributor

Great thanks, looks good to me!

@PowerUser64
Copy link
Contributor Author

Every 3 months is maybe more reasonable, unless a really important bug/security issue is fixed.

If you want to deal with nightly build, I would rather advice you create your own repository and to integrate it into NUR

Good to know. I'll try and limit updates to about one per 3 months, or when an official version number release comes out. There's supposedly going to be a major version update in the next few weeks, so I will probably do one then, and then who knows when the next official update will be.

@wackbyte
Copy link
Contributor

v1.2.0 has just released :)

@PowerUser64
Copy link
Contributor Author

Woohoo! New commit coming in hot!

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

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

2 packages built:
  • bespokesynth
  • bespokesynth-with-vst2

Result of nixpkgs-review pr 242698 run on x86_64-darwin 1

2 packages built:
  • bespokesynth
  • bespokesynth-with-vst2

Works on both systems, LGTM.

@luzpaz
Copy link
Contributor

luzpaz commented Aug 24, 2023

Any progress here ?

Copy link
Contributor

@OPNA2608 OPNA2608 left a comment

Choose a reason for hiding this comment

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

Ah sorry, seems I didn't review this too well…

In addition to the suggestion, can you make the commit title follow the commit conventions? It should be:

bespokesynth: 1.1.0 -> 1.2.0

…if you still only want to bump to the next stable version, instead of an unstable commit.

Once it LGTM this time, I'll try to find someone with merge permissions to get this in.

Comment on lines 67 to 68
rev = "71134ce432dd15ae773cd49ac7c6e436d37e8a46";
hash = "sha256-pEifqbzJYI0EnGRQL0a0hzNkUw4+ZE6mrMGUVngzTZY=";
Copy link
Contributor

Choose a reason for hiding this comment

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

I didn't pick up on the fact that when you force-pushed from bda87f7 to 179c1e2 , you went back to using an unstable commit without marking the version as such.

BespokeSynth/BespokeSynth@71134ce != https://github.com/BespokeSynth/BespokeSynth/releases/tag/v1.2.0 .

If this is still supposed to be version = "1.2.0" then please revert the rev line to the format that was used before and change hash to what the actual hash of that version is:

Suggested change
rev = "71134ce432dd15ae773cd49ac7c6e436d37e8a46";
hash = "sha256-pEifqbzJYI0EnGRQL0a0hzNkUw4+ZE6mrMGUVngzTZY=";
rev = "v${version}";
hash = "sha256-baBZBRK8xbtnj/MJaTA036exLjvvucfrsk/Ue5oiG7c=";

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sorry, I should have left a comment saying I did so. I updated to the latest commit because I learned the stable version had broken jedi functionality (python autocomplete) on MacOS. Since I don't know how long it was going to be until the next release (or opportunity to update this package), I went ahead and updated to the latest commit (which fixed this). I skipped updating the version number because I saw it as basically being a hotfix to v1.2.0, but I should probably mark it as unstable. Should I mark it as v1.2.0-unstable or would the date of the commit be better?

Copy link
Contributor

@tobiasBora tobiasBora Aug 25, 2023

Choose a reason for hiding this comment

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

It seems like most of the packages opted for v1.2.0+unstable=2023-07-20. I found 22 packages with this pattern, and it seems to be the version that complies the most with semantic versioning and its buildmetadata (otherwise, a dash indicates a pre-release, so happening before 1.2):

pkgs/tools/misc/tty-clock/default.nix
5:  version = "2.3+unstable=2021-04-07";

pkgs/tools/misc/dialogbox/default.nix
11:  version = "1.0+unstable=2020-11-16";

pkgs/applications/emulators/bsnes/higan/default.nix
25:  version = "115+unstable=2021-08-18";

pkgs/applications/emulators/emu2/default.nix
8:  version = "0.pre+unstable=2021-09-22";

pkgs/applications/graphics/gimp/plugins/default.nix
225:    version = "2.2+unstable=2021-12-03";

pkgs/applications/graphics/inkscape/extensions/applytransforms/default.nix
9:  version = "0.pre+unstable=2021-05-11";

pkgs/applications/terminal-emulators/st/siduck76-st.nix
15:  version = "0.pre+unstable=2021-08-20";

pkgs/applications/terminal-emulators/st/lukesmithxyz-st/default.nix
15:  version = "0.pre+unstable=2021-08-10";

…

Copy link
Contributor

Choose a reason for hiding this comment

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

I learned the stable version had broken jedi functionality (python autocomplete) on MacOS

If it's easy to figure out which commits fixed this, then you could pull them in and apply them onto 1.2.0:

patches = [
  # Fixes jedi functionality on Darwin
  # Remove when version > 1.2.0
  (fetchpatch {
    url = "https://github.com/BespokeSynth/bespokesynth/commit/whatevercommitfixesthis.patch";
    hash = "sha256-whateverhashisneededhere=";
  })
];

I skipped updating the version number because I saw it as basically being a hotfix to v1.2.0, but I should probably mark it as unstable

Unless it's an actual release with a tag (or in some other form actually treated by upstream as a release, with or without tag), it should be treated as an unstable commit instead of a release. For pulling in a fix into a release, see above.

Should I mark it as v1.2.0-unstable or would the date of the commit be better?

The above would be preferable IMO. If you want to ditch the applying-patches way, then back to what the manual says unstable versions should use: unstable-<YYYY-MM-DD of commit>

Copy link
Contributor

@OPNA2608 OPNA2608 Aug 25, 2023

Choose a reason for hiding this comment

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

It seems like most of the packages opted for v1.2.0+unstable=2023-07-20

This versioning pattern is from a not-yet-accepted RFC, dates used in your examples are from ~ when the RFC was first opened. And AFAICT they don't align with the format the RFC currently suggests to use: 3.0:date=2008-12-04.

I'd prefer to wait until any consensus is reached on that RFC & it gets accepted before using its pattern, as right now it conflicts with what the manual has to say on this.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Okay! When I get the chance, I'll update the version to unstable-2023-08-17 and the commit hash to c6b141 (slightly behind main). I think that's a good stable stopping place because most things added after it are features.
Does this sound good?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll also ask awwbees about making that into v1.2.1, as I think that would be a good place for a new version.

@PowerUser64
Copy link
Contributor Author

Okay, I've changed the version to unstable-2023-08-17 and updated to the commit I said I was going to update to (BespokeSynth/BespokeSynth@c6b1410).

Co-authored-by: Christoph Neidahl <[email protected]>
Co-authored-by: Tobias Bora <[email protected]>
@PowerUser64
Copy link
Contributor Author

Realized I forgot to update the commit message - just did it.

bespokesynth: 1.1.0 -> unstable-2023-08-17

@PowerUser64
Copy link
Contributor Author

What can I do to help get this merged?

@tobiasBora
Copy link
Contributor

You can drop a message on https://discourse.nixos.org/t/prs-already-reviewed/2617/726

@nixos-discourse
Copy link

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

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

@wegank wegank merged commit a34a81d into NixOS:master Sep 7, 2023
24 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

8 participants