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.
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; |
|
@dotlambda, thanks for the review—fixed! |
| buildInputs = [ | ||
| distutils_extra | ||
| setuptools-git | ||
| intltool |
There was a problem hiding this comment.
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.
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.)
| buildPythonApplication rec { | ||
| name = "screenkey-0.9"; | ||
|
|
||
| src = fetchFromGitHub { |
There was a problem hiding this comment.
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.
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.
Please do use pname and version since in long term buildPython* will likely get rid of name.
| buildInputs = [ | ||
| distutils_extra | ||
| setuptools-git | ||
| intltool |
| buildPythonApplication rec { | ||
| name = "screenkey-0.9"; | ||
|
|
||
| src = fetchFromGitHub { |
There was a problem hiding this comment.
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.
Have you considered patching instead?
There was a problem hiding this comment.
Do you mean patchelf? If so, there is no ELF to patch.
There was a problem hiding this comment.
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.
.so can be replaced with ${stdenv.hostPlatform.extensions.sharedLibrary}
|
@GrahamcOfBorg build screenkey |
There was a problem hiding this comment.
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.
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.
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.
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.
The icon themes do not need to be propagated, actually the hicolor_icon_theme is only used for setup hook.
|
|
||
| buildInputs = [ | ||
| defaultIconTheme | ||
| hicolor_icon_theme |
There was a problem hiding this comment.
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-sandboxinnix.confon non-NixOS)nix-shell -p nox --run "nox-review wip"./result/bin/)