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

Add a script to install tigervnc and remove tigervnc binaries from package #62

Closed
wants to merge 12 commits into from

Conversation

cmd-ntrf
Copy link
Contributor

@cmd-ntrf cmd-ntrf commented Sep 15, 2023

After reading jupyterhub/traefik-proxy code, I got inspired and decided to try to tackle #27 and #56. So I copied the traefik install script from jupyterhub/traefik-proxy, and made a few adjustments to download from Sourceforge.

I am marking this as draft as it not completed, but it works on my machine.

Also note, that I found out that starting with tigervnc 1.11.0, the vncserver script has been moved from bin to libexec.

@github-actions
Copy link

github-actions bot commented Sep 15, 2023

Binder 👈 Launch a binder notebook on this branch for commit e792a05

I will automatically update this comment whenever this PR is modified

Binder 👈 Launch a binder notebook on this branch for commit f56655c

Binder 👈 Launch a binder notebook on this branch for commit ec5043b

Binder 👈 Launch a binder notebook on this branch for commit be8ec8e

Binder 👈 Launch a binder notebook on this branch for commit 6135bbe

Binder 👈 Launch a binder notebook on this branch for commit 04dff5d

Binder 👈 Launch a binder notebook on this branch for commit 315348d

Copy link
Member

@manics manics left a comment

Choose a reason for hiding this comment

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

Can you add some docs to the README?

jupyter_remote_desktop_proxy/install.py Outdated Show resolved Hide resolved
parser.add_argument(
"--tigervnc-version",
dest="tigervnc_version",
default="1.10.0",
Copy link
Member

Choose a reason for hiding this comment

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

Does the current version 1.13.0 work?

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 have yet tested with 1.13.0. Starting at 1.11.0, tigervnc has moved vncserver from bin to libexec.

I have updated the PR to add the ability for jupyter-remote-desktop-proxy to find vncserver in either bin, libexec or if in user's PATH.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

So far, I have been unable to start a desktop with tigervnc > 1.10.0. vncserver was not only moved from bin to libexec but also deeply modified to the point where in tigervnc 1.13.0 it only accepts a display number.

But there might be hope. The vncserver perl script only purpose is to craft an Xvnc command-line with arguments and call it. Therefore, I think it might be possible to skip calling vncserver and call Xvnc instead since we already in the "command-line crafting business" with jupyter-remote-desktop-proxy. This assumes that Xvnc arguments were pretty much the same between tigervnc 1.10.0 and 1.13.0, which might be a strong hypothesis.

Copy link
Member

Choose a reason for hiding this comment

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

That's a pain, thanks for investigating. If it's easier then leave it as is, and we can come back to it in a future PR.

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 have added a list of valid choices for --tigervnc-version to avoid issue with users selecting anything above 1.10.1.

jupyter_remote_desktop_proxy/install.py Show resolved Hide resolved
@cmd-ntrf
Copy link
Contributor Author

Can you add some docs to the README?

I will gladly add some docs to the README, but I wanted to make sure first it was something we were interested in adding to jupyter-remote-desktop-proxy.

vncserver can be in bin or in libexec
@manics
Copy link
Member

manics commented Sep 27, 2023

I'm strongly in favour of having this. Bundling the binary VNCserver has several problems:

@yuvipanda
Copy link
Contributor

YESSS 1000% THIS IS AWESOME. I also would be ok with just ripping out the bundle anyway and requiring users to install it themselves via apt. The reason the traefik proxy has the install script was at that time traefik wasn't really available from any repositories...

@consideRatio
Copy link
Member

I also would be ok with just ripping out the bundle anyway and requiring users to install it themselves via apt.

I feel comfortable with removing special treatment for TigerVNC, but I'm hesitant to provide an install script because of the maintenance complexity long term without a deliberate decision to do so after evaluating pro/con for end users and long term maintenance. My understanding currently is that turbovnc is preferred, so why should we commit to helping install tigervnc specifically?

@manics
Copy link
Member

manics commented Feb 5, 2024

@yuvipanda Earlier you said "I also would be ok with just ripping out the bundle anyway and requiring users to install it themselves via apt."

Is this still the case? If so I think this would be good to sort out before the next (major?) release so we can resolve all licensing issues. This PR conflicts with your recent PRs so if we're just deleting share/tigervnc and updating the README to state a VNCserver must be installed it'll be easier to do that in a new PR rather than rebasing this one.

@yuvipanda
Copy link
Contributor

@manics yes, I think we should just rip it all out and just provide instructions for getting it from apt. We don't bundle xfce, we shouldn't bundle the vnc server either. I think we earlier did because the version from apt was too old, but I think that's not the case anymore.

@yuvipanda
Copy link
Contributor

@manics just to keep things going, I suggest we:

I know this makes it slower to resolve all the licensing issues, but I don't want to wait too long to get a release out and want to use the limited overall time we have more useful.

@cmd-ntrf
Copy link
Contributor Author

cmd-ntrf commented Feb 5, 2024

I am going to close this PR. Thank you everyone for the productive discussion.

giphy

@cmd-ntrf cmd-ntrf closed this Feb 5, 2024
@yuvipanda
Copy link
Contributor

Thank you very much for your work, @cmd-ntrf!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants