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

Jump to bottom

sonic-pi: 3.1.0 -> 3.2.0 #82043

Merged
merged 1 commit into from Jun 14, 2020
Merged

sonic-pi: 3.1.0 -> 3.2.0 #82043

merged 1 commit into from Jun 14, 2020

Conversation

c0deaddict
Copy link
Member

Motivation for this change

A new version of Sonic Pi has been released. The old version is currently broken because it depends on Ruby 2.4. This new version seems to work with Ruby 2.6.

Should this be merged in 20.03 as well so Sonic Pi can be used with that release?

I would like to become a maintainer of this package. I use it from time to time and am following the updates Sam Aaron (the creator) posts.

Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS linux)
  • Built on platform(s)
    • NixOS
    • macOS
    • other Linux distributions
  • Tested via one or more NixOS test(s) if existing and applicable for the change (look inside nixos/tests)
  • Tested compilation of all pkgs that depend on this change using nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
  • Tested execution of all binary files (usually in ./result/bin/)
  • Determined the impact on package closure size (by running nix path-info -S before and after)
  • Ensured that relevant documentation is up to date
  • Fits CONTRIBUTING.md.

@ofborg ofborg bot requested review from kamilchm and Phlogistique March 8, 2020 11:25
Phlogistique
Phlogistique approved these changes Mar 11, 2020
View reviewed changes
Copy link
Contributor

@Phlogistique Phlogistique left a comment

Choose a reason for hiding this comment

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

Looks good to me.

pkgs/applications/audio/sonic-pi/default.nix
make
mkdir build
pushd build
cmake -G "Unix Makefiles" ..
Copy link
Contributor

Choose a reason for hiding this comment

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

I know that is not changed in this PR, but shouldn't the call to CMake go in configurePhase? If you know a technical reason why it can not be in configurePhase, maybe it deserves a comment.

Copy link
Member Author

Choose a reason for hiding this comment

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

Thanks for your review!

I tried moving the CMake to the configurePhase, but it needs the /build/source/app/gui/qt/help_files.qrc that is build using the server/ruby/bin/qt-doc.rb script, which requires ruby.. and apparently buildInputs are not available in the configurePhase.

Copy link
Contributor

Choose a reason for hiding this comment

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

Wait, what? That breaks my understanding of Nix.

Copy link
Member Author

Choose a reason for hiding this comment

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

Scratch my comment about ruby not being present in the configurePhase, it is. I misread the logs. What is required is a file 2.6/ffi_c which appears to be compiled by ./compile-extensions.rb. It also needs the environment variables SONIC_PI_HOME and AUBIO_LIB ..

I agree that CMake should be in the configurePhase, but hauling in all the other stuff from the buildPhase feels wrong. What do you think?

Copy link
Member

Choose a reason for hiding this comment

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

Seems fine with me in the buildPhase

@Sohalt
Copy link
Contributor

Sohalt commented May 4, 2020

Tested an works for me, but there have been 2 minor releases since. Do you mind updating to v3.2.2?

@c0deaddict
Copy link
Member Author

Updated to 3.2.2

@ofborg ofborg bot requested a review from Phlogistique May 5, 2020 15:11
Sohalt
Sohalt approved these changes May 5, 2020
View reviewed changes
Copy link
Contributor

@Sohalt Sohalt left a comment

Choose a reason for hiding this comment

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

Works for me

@c0deaddict c0deaddict added the 9.needs: port to stable A PR needs a backport to the stable release. label May 7, 2020
@kimat
Copy link
Contributor

kimat commented May 31, 2020

I tried this and it builds at least, which is better than current master.

When running [nix-shell:~/.cache/nixpkgs-review/pr-82043]$ ./results/sonic-pi/bin/sonic-pi :
I can see the splash screen, but it crashes after with the following report : https://gist.github.com/kimat/cc11355bb521df8d3034e1d3527f41ed

I have a similar issue if I don't use nixpkgs-review and just checkout this branch and build with nix-build -A sonic-pi.

@Sohalt
Copy link
Contributor

Sohalt commented May 31, 2020

@kimat do you have a working Jack setup? SonicPi uses Jack for Audio output. I start my Jack server using qjackctl and then have SonicPi connect, which works fine for me.

@kimat
Copy link
Contributor

kimat commented May 31, 2020

@Sohalt It was an issue with jack indeed.

I can now, confirm the following works :

  • the package builds successfully using : nixpkgs-review pr 82043
  • the app itself runs using [nix-shell:~/.cache/nixpkgs-review/pr-82043-2]$ ./results/sonic-pi/bin/sonic-pi : I managed to get it to play audio.

@Lassulus
Copy link
Member

this was probably not introduced by this update, but there is a lot unnecessary files in the output path. like .md files .html a .gitignore a .travis.yml and even windows executables.
Would be nice If there would be less stuff which is not needed

@c0deaddict
Copy link
Member Author

Good point @Lassulus. I cleaned it up a bit and verified the app still works. Output went from 866M to 439M

@ofborg ofborg bot requested a review from Phlogistique June 13, 2020 13:08
@Lassulus Lassulus merged commit cdfb828 into NixOS:master Jun 14, 2020
This was referenced Jul 20, 2020
Open
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Lassulus Lassulus Lassulus left review comments

@Sohalt Sohalt Sohalt approved these changes

@kamilchm kamilchm Awaiting requested review from kamilchm

@Phlogistique Phlogistique Awaiting requested review from Phlogistique

Assignees
No one assigned
Labels
9.needs: port to stable A PR needs a backport to the stable release. 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@c0deaddict @Sohalt @kimat @Lassulus @Phlogistique