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

screenkey: 0.9 -> 1.2 #95437

Merged
merged 1 commit into from Aug 17, 2020
Merged

screenkey: 0.9 -> 1.2 #95437

merged 1 commit into from Aug 17, 2020

Conversation

doronbehar
Copy link
Contributor

Motivation for this change

Fix #89300 , cc @andys8 .

Things done
  • Use Python 3.
  • Switch to gobject-introspection and other more maintained libraries
    (vs PyGTK).
  • Use fetchFromGitLab.
  • Replace the non absolute paths fix in xlib.py with
    substituteInPlace.

cc @rasendubi .

  • 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 a review from rasendubi August 14, 2020 17:15
@doronbehar doronbehar force-pushed the pkg/screenkey branch 2 times, most recently from a1adc10 to 0fc2070 Compare August 14, 2020 20:41
jtojnar
jtojnar reviewed Aug 14, 2020
View reviewed changes
pkgs/applications/video/screenkey/default.nix
intltool

file
Copy link
Contributor

Choose a reason for hiding this comment

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

What is file used for?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

TBH without it, the build doesn't fail, but it spits:

sh: file: not found
sh: file: not found
sh: file: not found
sh: file: not found

So If you think it'd better without it, tell me.

Copy link
Contributor

Choose a reason for hiding this comment

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

It would be nice to find out what uses the file so we can document it in the comment.

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 had a slight feeling that it's related to distutils_extra, and I searched via the web interface at https://bazaar.launchpad.net/~registry/python-distutils-extra/debian/files but I couldn't find any evidence of this to be the cause :/. Perhaps we can hope that my comment mentioning https://gitlab.com/screenkey/screenkey/-/issues/122 is good enough?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

OK I added a comment stating that we don't know why file is needed, but without it there are "command not found" errors.

@doronbehar
Copy link
Contributor Author

Forgot to hit the "Comment" button yesterday when writing my comments to your review @jtojnar.

jtojnar
jtojnar reviewed Aug 17, 2020
View reviewed changes
pkgs/applications/video/screenkey/default.nix Outdated
Comment on lines 32 to 34
# This is here and not in buildInputs to avoid scrictDeps issue:
# https://github.com/NixOS/nixpkgs/issues/56943
gobject-introspection
Copy link
Contributor

Choose a reason for hiding this comment

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

It is actually the other way around. It belongs here but causes strictDeps issues. One way to “fix” it is to move it to buildInputs as you did before but that is a hack that has its own issues. I prefer disabling strictDeps instead, which is slightly less hacky.

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'm not sure I got to the bottom of this issue, but I moved gobject-introspection to buildInputs and disabled strictDeps.

Copy link
Contributor

Choose a reason for hiding this comment

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

I meant that it should remain in nativeBuildInputs (since that is the correct place) and strictDeps should be disabled (to bypass the bug).

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 meant that it should remain in nativeBuildInputs (since that is the correct place)

Really? Why?

Also, I didn't experience any issue running screenkey with either of the options I tested, with and without scrictDeps set. Still, now everything should be in place as you wish.

Copy link
Contributor

Choose a reason for hiding this comment

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

We add gobject-introspection for a setup hook so it should go to nativeBuildInputs. But that is currently broken with strictDeps = true (default in Python packages).

@doronbehar doronbehar force-pushed the pkg/screenkey branch 2 times, most recently from f0a7f52 to 5867f2a Compare August 17, 2020 15:07
jtojnar
jtojnar reviewed Aug 17, 2020
View reviewed changes
pkgs/applications/video/screenkey/default.nix Outdated
wrapGAppsHook
# https://github.com/NixOS/nixpkgs/issues/56943
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
# https://github.com/NixOS/nixpkgs/issues/56943
# for setup hook

Copy link
Contributor Author

Choose a reason for hiding this comment

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

👍.

@doronbehar
screenkey: 0.9 -> 1.2
b5caca2 
- Use Python 3.
- Switch to gobject-introspection and other more maintained libraries
(vs PyGTK).
- Use `fetchFromGitLab`.
- Replace the non absolute paths fix in `xlib.py` with
`substituteInPlace`.
jtojnar
jtojnar approved these changes Aug 17, 2020
View reviewed changes
Copy link
Contributor

@jtojnar jtojnar 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. Thanks.

@jtojnar jtojnar merged commit ebeefa8 into NixOS:master Aug 17, 2020
@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-ready-for-review/3032/301

@doronbehar doronbehar deleted the pkg/screenkey branch March 2, 2023 10:45
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jtojnar jtojnar jtojnar approved these changes

@rasendubi rasendubi Awaiting requested review from rasendubi

Assignees
No one assigned
Labels
10.rebuild-darwin: 0 10.rebuild-linux: 1-10 10.rebuild-linux: 1
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

screenkey: Update to 1.1
3 participants
@doronbehar @nixos-discourse @jtojnar