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.

Sign up for GitHub

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

megapixels: 0.16.0 -> 1.0.1 #121722

Merged
merged 1 commit into from
May 11, 2021
Merged

megapixels: 0.16.0 -> 1.0.1 #121722

merged 1 commit into from
May 11, 2021

Conversation

dotlambda
Copy link
Member

Motivation for this change

https://git.sr.ht/~martijnbraam/megapixels/refs/1.0.0
https://git.sr.ht/~martijnbraam/megapixels/refs/1.0.1

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.

I could not test this on my PinePhone yet.

@dotlambda dotlambda requested a review from OPNA2608 May 4, 2021 17:36
@ofborg ofborg bot added 11.by: package-maintainer This PR was created by the maintainer of the package it changes 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 May 4, 2021
OPNA2608
OPNA2608 reviewed May 4, 2021
View reviewed changes
pkgs/applications/graphics/megapixels/default.nix Outdated
++ optional jpgSupport graphicsmagick;
buildInputs = [
epoxy
gnome3.adwaita-icon-theme
Copy link
Contributor

Choose a reason for hiding this comment

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

Is that still correct with the bump to GTK4? I'm not that knowledgable about the GTK & GNOME world rn.

I only have a non-NixOS PinePhone OS, and adding that icon theme helped with making some icons show up properly. I don't know if fixing it like that was necessary / desirable when using a mobile NixOS installation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Actually, packages aren't supposed to bundle icon themes: https://nixos.org/manual/nixpkgs/unstable/#ssec-gnome-packaging

Copy link
Contributor

Choose a reason for hiding this comment

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

Oh, I didn't know that. Could you remove the icon theme as well then?

I'll build & test it on my PinePhone when I'm back from work today.

@dotlambda dotlambda requested a review from samueldr May 4, 2021 20:50
@ofborg ofborg bot requested a review from OPNA2608 May 5, 2021 08:52
OPNA2608
OPNA2608 approved these changes May 5, 2021
View reviewed changes
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.

It's broken on my postmarketOS system now but I assume it would work on a mobile NixOS one.

Large screengrab

grafik

@dotlambda dotlambda force-pushed the megapixels-1.0 branch 2 times, most recently from 98925d8 to a9c4ea0 Compare May 8, 2021 11:18
@dotlambda dotlambda requested review from zhaofengli and OPNA2608 May 8, 2021 11:20
@OPNA2608
Copy link
Contributor

OPNA2608 commented May 8, 2021

Adding libGL didn't fix it for me.

Terminal output

grafik

GL-related stuff has known problems on non-NixOS (nixGL didn't help), this'd prolly need to be tested on an actual mobile NixOS system?

For completeness, here are some strace logs.

megapixels.log
megapixels_nixGL.log

@dotlambda
Copy link
Member Author

I think the problem is simply #62169.

@dotlambda
Copy link
Member Author

this'd prolly need to be tested on an actual mobile NixOS system?

I won't be able to do that for at least a week.

@zhaofengli zhaofengli mentioned this pull request May 11, 2021
Merged
@zhaofengli
Copy link
Member

zhaofengli commented May 11, 2021 •
edited
Loading

Tested working on PinePhone running Mobile NixOS:

Screenshot

@dotlambda dotlambda merged commit c9494b6 into NixOS:master May 11, 2021
@dotlambda dotlambda deleted the megapixels-1.0 branch May 11, 2021 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@zhaofengli zhaofengli zhaofengli approved these changes

@samueldr samueldr Awaiting requested review from samueldr

@OPNA2608 OPNA2608 Awaiting requested review from OPNA2608

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 This PR does not cause any packages to rebuild on Darwin 10.rebuild-linux: 1-10 10.rebuild-linux: 1 11.by: package-maintainer This PR was created by the maintainer of the package it changes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@dotlambda @OPNA2608 @zhaofengli