-
-
Notifications
You must be signed in to change notification settings - Fork 15k
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
screenkey: init at 0.9 #34167
screenkey: init at 0.9 #34167
Conversation
pkgs/top-level/all-packages.nix
Outdated
@@ -4514,6 +4514,10 @@ with pkgs; | |||
quazip = quazip_qt4; | |||
}; | |||
|
|||
screenkey = callPackage ../applications/video/screenkey { |
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.
Better use python2Packages.callPackage
. Then you can leave out line 4518.
meta = with lib; { | ||
homepage = https://www.thregr.org/~wavexx/software/screenkey/; | ||
description = "A screencast tool to display your keys inspired by Screenflick"; | ||
license = licenses.gpl3; |
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.
This should be gpl3Plus
.
@dotlambda, thanks for the review—fixed! |
buildInputs = [ | ||
distutils_extra | ||
setuptools-git | ||
intltool |
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.
Why didn't you include all dependencies from https://www.thregr.org/~wavexx/software/screenkey/#installation-and-basic-usage?
Also, some of these should go into nativeBuildInputs
since they are only needed for building. See https://nixos.org/nixpkgs/manual/#ssec-stdenv-attributes.
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.
slop
is an optional runtime dependency and is not required for normal operation. (Even "install dependencies" command for Ubuntu/Debian does not list it.)
As for nativeBuildInputs
, I agree. (Developed the expression on top of the stable channel and it does not feature nativeBuildInputs
as a separate argument in mk-python-derivation
yet.)
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.
👍
buildPythonApplication rec { | ||
name = "screenkey-0.9"; | ||
|
||
src = fetchFromGitHub { |
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.
Why don't you use https://www.thregr.org/~wavexx/software/screenkey/releases/screenkey-0.9.tar.gz?
Also, you could specify pname
and version
and leave out name
.
Sorry for the many comments :)
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.
IDK, fetchFromGitHub
is more convenient to use. GitHub is also less likely to die or move.
Don't see a point in using pname
and version
—I am using name
as rev
(and reconstructing it from pname
and version
would remove all the benefit)
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.
Alright, as you wish.
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.
Please do use pname
and version
since in long term buildPython*
will likely get rid of name
.
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.
ok, fixed
buildInputs = [ | ||
distutils_extra | ||
setuptools-git | ||
intltool |
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.
👍
buildPythonApplication rec { | ||
name = "screenkey-0.9"; | ||
|
||
src = fetchFromGitHub { |
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.
Please do use pname
and version
since in long term buildPython*
will likely get rid of name
.
]; | ||
|
||
makeWrapperArgs = [ | ||
"--set LD_LIBRARY_PATH ${libX11}/lib:${libXtst}/lib" |
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.
Have you considered patching instead?
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.
Do you mean patchelf
? If so, there is no ELF to patch.
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.
You would create paths.patch
--- a/Screenkey/xlib.py
+++ b/Screenkey/xlib.py
@@ -6,7 +6,7 @@
from ctypes import *
## base X11
-libX11 = CDLL('libX11.so.6')
+libX11 = CDLL('@libX11@/lib/libX11.so.6')
# types
Atom = c_ulong
@@ -278,7 +278,7 @@
## record extensions
-libXtst = CDLL('libXtst.so.6')
+libXtst = CDLL('@libXtst@/lib/libXtst.so.6')
# types
XPointer = String
Then you would add
{
patches = [
(substituteAll {
src = ./paths.patch;
inherit libX11 libXtst;
})
];
}
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.
.so
can be replaced with ${stdenv.hostPlatform.extensions.sharedLibrary}
@GrahamcOfBorg build screenkey |
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.
Failure for system: x86_64-darwin
Package ‘screenkey-0.9’ in /Users/graham/nix-borg/repo/38dca4e3aa6bca43ea96d2fcc04e8229/builder/grahamc-zoidberg/pkgs/applications/video/screenkey/default.nix:38 is not supported on ‘x86_64-darwin’, refusing to evaluate.
a) For `nixos-rebuild` you can set
{ nixpkgs.config.allowBroken = true; }
in configuration.nix to override this.
b) For `nix-env`, `nix-build`, `nix-shell` or any other Nix command you can add
{ allowBroken = true; }
to ~/.config/nixpkgs/config.nix.
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.
Success for system: x86_64-linux
reading manifest template 'MANIFEST.in'
warning: no previously-included files found matching '.gitignore'
writing manifest file 'screenkey.egg-info/SOURCES.txt'
running build_ext
----------------------------------------------------------------------
Ran 0 tests in 0.000s
OK
/nix/store/n55akhflgwfcvr9636hzv5akqh4kir09-screenkey-0.9
No tests are found. Either the test runner cannot find tests and needs to be patched, or there simply are no tests in which case the tests need to be disabled. Do include a comment explaining why the tests are disabled. |
There are no tests. Why should the check phase be disabled? It is not failing and runs within a second so it does no harm. Furthermore, if upstream adds tests, it's very likely the check phase won't be re-enabled. Am I missing something? |
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.
Success for system: aarch64-linux
reading manifest template 'MANIFEST.in'
warning: no previously-included files found matching '.gitignore'
writing manifest file 'screenkey.egg-info/SOURCES.txt'
running build_ext
----------------------------------------------------------------------
Ran 0 tests in 0.000s
OK
/nix/store/rvqy26hbac7xy798z1kiavjy85hnzgj8-screenkey-0.9
Replaced the wrapper hack with a patch to screenkey. @GrahamcOfBorg build screenkey |
Success on x86_64-linux (full log) Partial log (click to expand)
|
Success on aarch64-linux (full log) Partial log (click to expand)
|
The comment should be added in order not to confuse other contributors. When I see "Ran 0 tests in 0.000s", I sometimes try to look up what the correct |
Success on x86_64-linux (full log) Partial log (click to expand)
|
Success on aarch64-linux (full log) Partial log (click to expand)
|
Apparently, we already have screenkey in |
nativeBuildInputs = [ | ||
distutils_extra | ||
setuptools-git | ||
intltool |
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.
Adding wrapGAppsHook
in nativeBuildInputs
and defaultIconTheme
, hicolor_icon_theme
in buildInputs
should provide icons regardless of the desktop environments. The other pull request for screenkey seems to do something similar: #34898
Success on x86_64-linux (full log) Partial log (click to expand)
|
propagatedBuildInputs = [ | ||
pygtk | ||
|
||
defaultIconTheme |
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.
The icon themes do not need to be propagated, actually the hicolor_icon_theme
is only used for setup hook.
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.
Oops. Wrong section.
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.
Fixed.
|
||
buildInputs = [ | ||
defaultIconTheme | ||
hicolor_icon_theme |
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.
Maybe also add comment that it is added for setup hook – or remove it, since it is propagated by adwaita-icon-theme
.
Success on x86_64-linux (full log) Partial log (click to expand)
|
Failure on aarch64-linux (full log) Partial log (click to expand)
|
Ok, guys. I went through, like, 6-8 review rounds, and I have little energy to move it forward this way. If there are no blockers, I would like to get this merged as is. If you have any improvements in mind, feel free to push into my branch. |
Failure on aarch64-linux (full log) Partial log (click to expand)
|
sure thing... I'm on it now
…On Mon, Feb 12, 2018 at 06:07:44PM +0000, Alexey Shmalko wrote:
@Mic92, updated to include icons.
@jtojnar, I removed `pythonPackages.screenkey`. (Don't think it deserves an alias as I believe nobody is using it.)
@vidbina would you like to review/test as well?
--
You are receiving this because you were mentioned.
Reply to this email directly or view it on GitHub:
#34167 (comment)
|
@rasendubi, on my setup this fails with
It fails on the XopenIM call which I resolved in my old PR ( #34898) by unsetting I proposed a PR to this PR to keep some work of @rasendubi's shoulders. I understand it's been much 😅, so @jtojnar and @dotlambda please weigh in on this and provide some feedback. I'm not familiar with unsetting variables using the |
Using
|
@jtojnar, yeah other GTK 2 apps work for me. In my case Enabling an input method sets Between input methods such as fcitx, ibus, nabi or uim and the method Several threads online proposed the It probably won't win the pageant but it is a pragmatic fix for those who use non-latin input methods. |
Motivation for this change
Things done
build-use-sandbox
innix.conf
on non-NixOS)nix-shell -p nox --run "nox-review wip"
./result/bin/
)