techahold / rustdeskinstall

Easy install Script for Rustdesk
MIT License
357 stars 110 forks source link

A few suggestions that might #4

Closed philclifford closed 2 years ago

philclifford commented 2 years ago

Just some completely untested notes so setting this draft.

dinger1986 commented 2 years ago

Shall have a proper look but looks like the option of using an IP has been removed?

dinger1986 commented 2 years ago

Guess checking for systemd is necessary as well. However should just change the description to modern os

dinger1986 commented 2 years ago

Typos and sudo due to late night dev! Shall sort them.

Just noticed I'm just up the road from you, in Perthshire.

philclifford commented 2 years ago

I do that a lot too - my missing fi contribution was due to the summons to eat :-) I enjoyed Perthshire when we used it as a base for the Glasgow Commonwealth Games combined with a holiday that was probably the start of our journey to becoming "New Scots" ( although it seems I've really just come "home".

Shall have a proper look but looks like the option of using an IP has been removed?

? :eyeglasses: ?

dinger1986 commented 2 years ago

Lol. So not actually in Ayrshire then?

In the first change on the install script there's a check for a genuine domain, I don't believe the automatic retrieval of IP has been left in. As some will use dns addresses and some are happy using IP, I wanted to give the opportunity to use both, especially at the moment as not using ssl for anything.

dinger1986 commented 2 years ago

nope IP check is still there, thats what I get for checking on my phone lol

dinger1986 commented 2 years ago

going to merge into dev when you have completed the draft from your side

philclifford commented 2 years ago

Lol. So not actually in Ayrshire then? Oh yes - East Ayrshire and loving it. I'll try and test it out later and if I didn't break anything obvious I'll de-draft it.

dinger1986 commented 2 years ago

Ah cool, Ill test off of your repo now

dinger1986 commented 2 years ago

ok, so tested on 3 distros centos: ./install.sh: line 13: dig: command not found (easy sorted just need a yum install dig at the start) Installing prerequisites Unsupported OS

ubuntu 20.04: (ran prereqs manually and installed fine) unzip command doesnt exist

Debian 11: Choose your preferred option, IP or DNS/Domain:1 Installing prerequisites Unsupported OS

philclifford commented 2 years ago

Thanks for the feedback - I'll take a look at what I've borked ...

dinger1986 commented 2 years ago

I don't have the energy myself to debug tonight but after meetings tomorrow can have a look and do some tests etc depending how you get on

philclifford commented 2 years ago

So far so good : tested and working on ZorinOS and Pop! and Mint ... and debian 11.4 and centos-stream 9.0

Fails to fly on opensuse for some reason ...

export DEBUG=true to stop with some diagnostics after the OS detection. You can then see what new and interesting interpretation of "standard" has enabled the distro under test to demonstrate their USP and extend the code accordingly

Enough for now ... should work on rhel, debian and ubuntu derivatives OK. I'll leave arch support to others btw, also *bsd and NixOS :-)

philclifford commented 2 years ago

ok, so tested on 3 distros centos: `./install.sh: line 13: dig: command not found (easy sorted just need a yum install dig at the start)

centos-stream 9 didn't need dig, but maybe ... probably worth adding to the prereqs and make that the first action

tested on alma and failed because no wget ??? manually installing prereqs first it sailed through OK so there's something fishy there (it looks like it gets picked up as a rhel derivative but then messes up the yum somehow ... probably the same issue with opensuse ...)

dinger1986 commented 2 years ago

yeah move os detect and prereqs first I think would make sense.

Can deal with any anomalies as they come up with distros, ill test with centos, ubuntu and debian, tbf the majority of first time users/guys with less experience will use one of those 3 I would imagine.

dinger1986 commented 2 years ago

is that you finished now? If so can merge to dev

dinger1986 commented 2 years ago

still getting the issues using a vps hosted on hetzner, how are you testing it?

Debian - E: Unable to locate package curl wget unzip tar dnsutils

philclifford commented 2 years ago

local QEMU VMs Not always freshly installed (some are playing at being github workflow runners etc ...

dinger1986 commented 2 years ago

how strange, moved the install above the choice so dig is definitely installed, just playing with it myself, I have seen combined before not working for apt and yum

philclifford commented 2 years ago

still getting the issues using a vps hosted on hetzner, how are you testing it?

Debian - E: Unable to locate package curl wget unzip tar dnsutils

oof! Hmm, my vm must have been pre-populated.

I think it is getting quoted combined package name to install :-( Either need to separate out again (ugly) or refactor to "+=" the whole command or some such.

dinger1986 commented 2 years ago

I have changed it slightly and it appears to work fine now, but thats separating out the prereqs, which doesnt make much sense why it would be different other than the system recognises them as different package names

philclifford commented 2 years ago

Suse was me forgetting yast/zypper since a long time ... working on a refactor but can't test til later.

philclifford commented 2 years ago

I think you just need to remove the quotes from apt-get install ${PREREQ}

dinger1986 commented 2 years ago

done that and yeah working fine, I broke debian prereqs off to PREREQDEB and centos to PREREQYUM just incase we need to use it more in the future

philclifford commented 2 years ago

see alternate PR ... I need to get to other stuff now ... back later