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
surge: init at 1.7.1 #82308
surge: init at 1.7.1 #82308
Conversation
Thanks for the review. All done. |
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.
Looks good to me, other than the nit.
@puzzlewolf Thanks for the feedback. All done. |
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.
Sorry for piecemeal reviews :) I found some more things.
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.
@magnetophon Do the tests work for you like that? Do you think that's a good solution?
@baconpaul Thank for the pointers!
mkdir -p $out/share/surge | ||
cp -r resources/data/* $out/share/surge/ | ||
''; | ||
|
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.
doInstallCheck = true; | |
installCheckPhase = '' | |
./build-linux.sh build -p headless | |
./target/headless/Release/Surge/Surge-Headless | |
''; |
I made the tests work :) They're install checks because surge-headless
looks for the resources in the install directory.
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.
Yeah that makes sense - there’s no reason to run them any time other than after build.
A couple of the tests have tolerance problems I’m chasing and will fail about one time in 20 just FYI.
@puzzlewolf @baconpaul Thanks! Now all we need is a way to make zenity work without installing it globally or in the user profile.
but that gave a build error. |
Yeah, substituteInPlace src/linux/UserInteractionsLinux.cpp --replace 'execlp("zenity"' 'execlp("${gnome3.zenity}/bin/zenity"'
substituteInPlace src/common/gui/PopupEditorDialog.cpp --replace zenity ${gnome3.zenity}/bin/zenity Can you check if it actually works? Even if it works, I really don't know if this is a good idea. |
@puzzlewolf That works wonderfully, thank you!
|
Folks: I am more than happy to take changes to upstream so you don't have to do these substitutions to build (in fact I am way way happier to take changes upstream). For instance we could easily make a quick function which gets the name of the zenity executable in the C++ and defaults to 'zenity' but reads an environment variable. And even have it conditionally build a different version. I would merge that PR happily. Would even put it on my queue if you want me to do it if you just open up an issue over on the surge repo. The more that we have visibility on changes made to the code in master, even if it ends up being a bit of spaghetti of ifs, the less likely it is we will break you when we have a new release. etc... you know the drill. |
An obvious change, for instance, now that I have moved master to 100% cmake, is to have some cmake rule that detects nixos build and adds a -DNIXOS and then change the zenity symbol based on that. Is there some way at build time that you indicate I am in your environment? (And also are you building master or 1.6.6? like I said, master moved entirely to cmake this week) |
@baconpaul That's very generous of you, but my C/C++ fu is lacking, and I'd hate to saddle you up with more work on Surge that only benefits a few users.
This PR is building 1.6.6. |
Great! So can we merge this after a month+? =) |
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.
Yes 👍
I have updated the package definition after the switch to cmake (which simplified it a lot). |
eae98b6
to
2449839
Compare
Thanks! |
Motivation for this change
Things done
sandbox
innix.conf
on non-NixOS linux)nix-shell -p nixpkgs-review --run "nixpkgs-review wip"
./result/bin/
)nix path-info -S
before and after)