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.

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

x11docker: init at 5.4.1 #55723

Merged
merged 2 commits into from Feb 16, 2019
Merged

x11docker: init at 5.4.1 #55723

merged 2 commits into from Feb 16, 2019

Conversation

jD91mZM2
Copy link
Member

Motivation for this change
Things done
  • Tested using sandboxing (nix.useSandbox on NixOS, or option sandbox in nix.conf on non-NixOS) (I think, it says this is the default)
  • 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 nox --run "nox-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)
  • Assured whether relevant documentation is up to date
  • Fits CONTRIBUTING.md.

This adds a recipe for x11docker, and also the nxagent backend because it supports both seamless and desktop mode.

@averelld
Copy link
Contributor

Just nuking nxproxy would break x2goclient. Ideally we could get multiple outputs from this build, similar to https://git.archlinux.org/svntogit/packages.git/tree/trunk/PKGBUILD?h=packages/nx

@jD91mZM2
Copy link
Member Author

IIRC, there is a separation of dev/bin outputs possible, maybe I can use it to further difference outputs of nxagent/nxproxy/libs? I'll check it out later

@Ma27
Copy link
Member

Ma27 commented Feb 13, 2019

You also need to patch shebangs, it currently fails with the following error locally:

./result/bin/x11docker-gui: /nix/store/xlai4ls0n42jml470pkrxp7cmr56rj4i-x11docker-5.4.1/bin/.x11docker-gui-wrapped: /bin/bash: bad interpreter: No such file or directory
./result/bin/x11docker-gui: line 3: /nix/store/xlai4ls0n42jml470pkrxp7cmr56rj4i-x11docker-5.4.1/bin/.x11docker-gui-wrapped: Success

@symphorien
Copy link
Member

You also need to patch shebangs

this will come for free by not using phases = which disables the default fixupPhase.

@jD91mZM2
Copy link
Member Author

Right, I forgot to check the gui. Thanks for the dontBuild tip!

@jD91mZM2
Copy link
Member Author

The gui fails with

Output of `x11docker-gui` 
Usage: grep [OPTION]... PATTERN [FILE]...
Try 'grep --help' for more information.
/nix/store/q3ka7ydgcx2ngwyhxf705a3c9nm38jq5-x11docker-5.4.1/bin/.x11docker-gui-wrapped: line 119: command: --: invalid option
command: usage: command [-pVv] command [arg ...]
sed: can't read /usr/share/X11/xkb/rules/base.lst: No such file or directory
x11docker-gui: Did not find executeable 'kaptain'.
If your distribution does not provide package kaptain (>=0.73), look at
kaptain repository:   https://github.com/mviereck/kaptain
Fallback: Will try to use image x11docker/kaptain.

/nix/store/q3ka7ydgcx2ngwyhxf705a3c9nm38jq5-x11docker-5.4.1/bin/.x11docker-gui-wrapped: line 837: export: `Options:': not a valid identifier
/nix/store/q3ka7ydgcx2ngwyhxf705a3c9nm38jq5-x11docker-5.4.1/bin/.x11docker-gui-wrapped: line 839: /share/stdin: No such file or directory

It appears we need to package kaptain for that gui, so why not just disable that for now? The command line client is (at least to me) the most interesting part of x11docker.

@Ma27
Copy link
Member

Ma27 commented Feb 15, 2019

I'll see if I have enough time to test the package today. If the x11docker tool works fine, I'd say that we could merge this (but we shouldn't move the GUI part into the store path and add a TODO comment instead).

@mviereck
Copy link

mviereck commented Feb 15, 2019 •
edited

I am the developer of x11docker, and I stumbled over this thread yet.
Thank you, I am honoured that you like to integrate x11docker into NixOS!

If there is anything needed to adjust x11docker for NixOS, please tell me. I once did some basic test runs in a NixOS VM, but did not check out all possible setups.

It appears we need to package kaptain for that gui, so why not just disable that for now? The command line client is (at least to me) the most interesting part of x11docker.

I am currently looking at the x11docker-gui issues mentioned above. However, the interesting part is the x11docker script itself, and it needs no other files.

x11docker-gui runs as well with image x11docker/kaptain. I have the impression that it is used more often than a native kaptain installation.

@jD91mZM2
Copy link
Member Author

jD91mZM2 commented Feb 15, 2019 •
edited

The docker image didn't seem to work, or at least that's what I assume the error is. Cool thing you stumbled upon this so quickly, I was going to email you once this was merged to let you know your program got packaged. It's a really awesome tool and I'm excited to start packaging some proprietary X11 apps :)

@Ma27
Copy link
Member

Ma27 commented Feb 15, 2019

If there is anything needed to adjust x11docker for NixOS, please tell me. I once did some basic test runs in a NixOS VM, but did not check out all possible setups.

I didn't read through the entire x11docker-gui script, but I guess that most of the problems occur since NixOS isn't fully FHS-compliant and e.g. doesn't have a standard /usr directory (except for /usr/bin/env) because of its special behavior.

Most of the occurrences can be solved by a simple substitute(InPlace). I don't know though, how much effort it'll take to package kaptain.

@Ma27
nx-libs: minor fixes
346df14 
* Add an alias with a deprecation warning for `nxproxy` to avoid an
  immediate breaking change.

* Use the default shell used in the build environment (`stdenv.shell`)
  for patching. This shell is in the environment and thus used to patch
  scripts using `patchShebangs`. The shell is referenced as `stdenv.shell`
  in Makefiles to patch the remaining occurrences of `/bin/bash` in the
  build environment.
@Ma27
Copy link
Member

Ma27 commented Feb 16, 2019

I pushed a simple commit to your branch that fixes my review. If it's fine for you, I'd go ahead and merge as I just tested x11docker and it seems to work fine 👍

@jD91mZM2
Copy link
Member Author

Ignore my review (which I deleted), I didn't realize stdenv.shell was always bash. Looks awesome, thanks for the commit! I'm very fine with merging this right now 😄

@Ma27 Ma27 merged commit 3784198 into NixOS:master Feb 16, 2019
@Ma27
Copy link
Member

Ma27 commented Feb 16, 2019

@jD91mZM2 thanks!

@srhb srhb mentioned this pull request Feb 16, 2019
Merged
10 tasks
@vcunat
Copy link
Member

vcunat commented Feb 16, 2019

Cross-ref: 10f37a3.

vcunat added a commit that referenced this pull request Feb 16, 2019
@vcunat
nxproxy: drop the warning introduced by #55723
10f37a3 
The tarball job fails when warnings are detected (and blocks channel).
And that's good, because `nix-env -qa` also gets these warnings.

I'm afraid we still don't have a good way to deprecate attributes,
exactly because the inability to distinguish these "listing actions"
from explicit usage (direct or transitive).
@Ma27
Copy link
Member

Ma27 commented Feb 16, 2019

@vcunat thanks a lot for fixing this and the elaboration in your commit message. I guess I was wrong here, so I'm sorry!

Just out of interest, are there any proposals on how to properly deprecate attribute names (like a package in this case?)

@vcunat
Copy link
Member

vcunat commented Feb 16, 2019

The most recent thread I know: NixOS/rfcs#33

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@Ma27 Ma27 Ma27 left review comments

@symphorien symphorien symphorien left review comments

Assignees
No one assigned
Labels
8.has: clean-up 8.has: documentation 8.has: package (new) 10.rebuild-darwin: 0 10.rebuild-linux: 1-10 11.by: package-maintainer
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
7 participants
@jD91mZM2 @averelld @Ma27 @symphorien @mviereck @vcunat @GrahamcOfBorg