winapps-org / winapps

The winapps main project, forked from https://github.com/Fmstrat/winapps/
Other
998 stars 45 forks source link

Several Fixes (freerdp3 command line, able to be run on multiple distros, fixing a typo) #87

Closed GreatNovaDragon closed 4 months ago

GreatNovaDragon commented 5 months ago

… to install all officially supported apps in one go, also fix a typo

This PR contains Several Fixes. With FreeRDP3, the command line interface has been reworked, making the old one non-compatible.

Several of the commands given here made the baseline assumption that it is run on Ubuntu, like the assumption the command is always named "xfreerdp", when at least under Arch it is xfreerdp3. So the check for that is also in, and the option to supply an own command there. Or the base assumption that bc is installed (it isnt under arch)

It also adds an convenience argument to setup all officially supported apps at once.

Also, there was an typo for the Windows launcher, telling us it belonged to "Micorosoft" instead of Microsoft

CLAassistant commented 5 months ago

CLA assistant check
All committers have signed the CLA.

GreatNovaDragon commented 5 months ago

Should fix #81 Might fix #79 Affects #69 in that you would be able to just select the commands for sdl-freerdp3 and wlfreerdp3 (even if their support for RemoteApp is not existent right now)

oskardotglobal commented 5 months ago

Hello, thank you for your contribution. So far this looks good, but I will have to test it on my machine

oskardotglobal commented 5 months ago

@LDprg Thoughts?

LDprg commented 5 months ago

Seems very good to me, a lot of great refinements. Thx for the contribution.

@oskardotglobal have you tested it?

However we also should make a proper review. (I will do one propably by the end of this week, if nobody does before)

LDprg commented 5 months ago

@oskardotglobal What is about our licence issue? Are we even permitted to merge this pr.

oskardotglobal commented 5 months ago

@oskardotglobal What is about our licence issue? Are we even permitted to merge this pr.

Well, we're technically not allowed to redistribute or modify winapps in any way, but given that fmstrat isn't going to do anything about it, we're fine. Also, as the PR's author has signed the CLA its changes fall under the GPLv3 license, so at least that part is all clear

LDprg commented 4 months ago

@AkechiShiro have you tested this pr?

I would like to merge this as soon as possible (of course if tested properly).

AkechiShiro commented 4 months ago

I haven't found the time to do so sorry @LDprg

Would you be able to test it ? I can try maybe next week

LDprg commented 4 months ago

I am also quite busy @AkechiShiro, not sure when I can test this. @oskardotglobal maybe you could take a look?

I would suggest anyone testing first just reports their find here. The worst (or best ;-) ) that could happen is that we have multiple reports at the same time.

AkechiShiro commented 4 months ago

It's bit annoying to test we need to setup a W11 VM manually using KVM no ?

I spawned a Garuda Linux VM on my side, but I may need to install W11 VM to test it correctly (I'm on NixOS and this update is for the old code of winapps not the new rewrite Nix friendly, I believe)

AkechiShiro commented 4 months ago

Maybe I'm doing something wrong but running the installer.sh script shows there might be syntax errors : image

Can you test the script please ? @GreatNovaDragon ?

Or wait, this is an issue somewhere else in another script ? Not in the installer.sh one...?

oskardotglobal commented 4 months ago

@AkechiShiro you can always use quickemu to set up the VM I will see if I can get this to work on my Fedora machine too

AkechiShiro commented 4 months ago

@oskardotglobal just finished setting up one using quickemu thanks for the mention !

AkechiShiro commented 4 months ago

Ran into this issue : https://github.com/quickemu-project/quickemu/issues/686

AkechiShiro commented 4 months ago

My laptop is rebooting when I run a VM inside a VM wasn't expecting this one...

I'll need to test on another laptop running natively ArchLinux, I will be able to most likely in 2 weeks.

LDprg commented 4 months ago

I will start writing a docker guide at some point for the legacy winnapps (and code for the rewrite). Docker should work prevent most of the issue with quickemu (because it is specifically build for portability and windows).

oskardotglobal commented 4 months ago

Yeah, I'm also kinda fed up with quickemu Using Docker will help here

oskardotglobal commented 4 months ago

Alright, I'm gonna test it right now

oskardotglobal commented 4 months ago

Ok, so I've caught a syntax error which caused me to not be able to run the script, but I've fixed that

oskardotglobal commented 4 months ago

Unless anyone else has anything to say, I say this is ready to merge

oskardotglobal commented 4 months ago

Maybe I'm doing something wrong but running the installer.sh script shows there might be syntax errors : image

Can you test the script please ? @GreatNovaDragon ?

Or wait, this is an issue somewhere else in another script ? Not in the installer.sh one...?

I also fixed that

AkechiShiro commented 4 months ago

Thanks a lot @oskardotglobal and sorry for not having been able to wholefully test it

EDIT : The issue I ran into during testing on my laptop => https://community.frame.work/t/responded-hard-resets-running-vms-on-amd-7640u/46442/15

I've got no luck XD