-
-
Notifications
You must be signed in to change notification settings - Fork 13.6k
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
sozi: init at 23.7.25-1690311612 #321465
sozi: init at 23.7.25-1690311612 #321465
Conversation
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.
Thanks for this PR, @srghma, much appreciated.
Please find below a few ideas and suggestions on how to improve this PR. Hopefully I've been able to give understandable and helpful feedback. Do let me know if there are any questions.
Thank you for accepting the review suggestions, @srghma, much appreciated 👍 Can you please squash all commits into a single one. |
If you wanted to go above and beyond you could also format the file using nixfmt, e.g. |
e8863ac
to
16eb5d7
Compare
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.
Thanks for the contribution! Some feedback below:
Also per CI, the URL doesn't seem to exist?
|
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.
Also, please squash the commits + rebase your branch on a recent commit, 171a116 is from October 2023 and is the reason why you were seeing a lot of strange behaviour.
b0cada8
to
7f31e2c
Compare
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.
Thanks for addressing all the review comments, @srghma, very much appreciated. What's your take on the following?
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.
SVG files appear to open fine, lgtm
install -m 444 -D ${appimageContents}/sozi.desktop -t $out/share/applications | ||
cp -r ${appimageContents}/usr/share/icons $out/share | ||
substituteInPlace $out/share/applications/sozi.desktop \ | ||
--replace 'Exec=AppRun' 'Exec=sozi' |
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.
--replace 'Exec=AppRun' 'Exec=sozi' | |
--replace-fail 'Exec=AppRun' 'Exec=sozi' |
as --replace
is deprecated
Description of changes
adding sozi
Things done
nix.conf
? (See Nix manual)sandbox = relaxed
sandbox = true
nix-shell -p nixpkgs-review --run "nixpkgs-review rev HEAD"
. Note: all changes have to be committed, also see nixpkgs-review usage./result/bin/
)Add a 👍 reaction to pull requests you find important.