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

Simplify installation process #232

Merged
merged 11 commits into from
Sep 7, 2024
Merged

Simplify installation process #232

merged 11 commits into from
Sep 7, 2024

Conversation

oskardotglobal
Copy link
Member

This PR does the following:

  • installer.sh is renamed to setup.sh
  • The setup.sh and bin/winapps are now symlinked to $BIN_PATH
  • It's checked that $BIN_PATH is on $PATH
  • The script also now clones the repo to a default location in ~/.local/bin/winapps

Copy link
Member

@LDprg LDprg left a comment

Choose a reason for hiding this comment

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

Great, this would simplify alot and prevent a lot of problems related to local git repos.

README.md Show resolved Hide resolved
@oskardotglobal oskardotglobal marked this pull request as ready for review August 29, 2024 10:32
@oskardotglobal oskardotglobal linked an issue Aug 29, 2024 that may be closed by this pull request
Copy link
Member

@KernelGhost KernelGhost left a comment

Choose a reason for hiding this comment

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

These are some fantastic and welcomed changes. Unfortunately my computer is out of order currently so I have not been able to test these changes. Based on a cursory inspection, LGTM.

@oskardotglobal
Copy link
Member Author

@LDprg Could you test this on your system?

@LDprg
Copy link
Member

LDprg commented Aug 30, 2024

@LDprg Could you test this on your system?

I will take a look soon, but I am currently very low on time.

@AkechiShiro
Copy link

AkechiShiro commented Sep 3, 2024

On which Distro (I'm daily driving NixOS at the moment) would you like me to test this @oskardotglobal (Debian or ArchLinux ?) looking to move forward #234

@oskardotglobal
Copy link
Member Author

I also run Nix and I've tested it there, but if you test it on another distro try Debian

@AkechiShiro
Copy link

Ran into this issue after installing deps (curl and dialog) @oskardotglobal
image

@AkechiShiro
Copy link

I think the issue comes from here :
image

@AkechiShiro
Copy link

Removing readonly doesn't seem to fix it :
image

@oskardotglobal
Copy link
Member Author

@AkechiShiro This should fix your problem

@AkechiShiro
Copy link

AkechiShiro commented Sep 7, 2024

Seem to have an issue again @oskardotglobal

I don't think you're checking if the current user is in sudoers or wheel group also what if user is using something else than sudo like doas ?

I'm just trying it as a total Linux newbie to make sure we squash easy stuff early on.

image

Current user + manual => currently testing.

@AkechiShiro
Copy link

AkechiShiro commented Sep 7, 2024

Didn't follow the steps properly but I think we should create the config file by default instead of asking the user to do another step :
image

Should I reinstall or uninstall ? The installation state is not clear right now

@AkechiShiro
Copy link

I think dependency checks should be done at the beginning, I've had to install 2 deps before starting the script and now it is asking for more :
image

I can help on reworking the PR to make it better if needed

@AkechiShiro
Copy link

This dependency does not exist on Debian :
image

@oskardotglobal
Copy link
Member Author

These issues are not related to this PR, but need addressing anyways

@AkechiShiro
Copy link

AkechiShiro commented Sep 7, 2024

Seems freerdp3 is not available on Debian...

I probably need to enable backports from unstable branches : https://packages.debian.org/bookworm-backports/freerdp3-x11

This will need documentation, opened #243

@oskardotglobal
Copy link
Member Author

Either that or use Flatpak, but that's kind of broken
We need a solution for this, ideally we'd be able to ship FreeRDP with winapps

@oskardotglobal
Copy link
Member Author

Or you know, just tell everyone to install the Nix package manager and install WinApps through there

@AkechiShiro
Copy link

We both know that's not gonna work out XD, but someday maybe Nix will become mainstream

@AkechiShiro
Copy link

We can ship Nix with Winapps and have it pull freerdp3

@AkechiShiro
Copy link

@oskardotglobal
Copy link
Member Author

We both know that's not gonna work out XD, but someday maybe Nix will become mainstream

We can always dream ;D

Nix-bundle seems like a good idea, we can experiment with that

@oskardotglobal
Copy link
Member Author

Actually, it seems like not that straight forward. It seems we'd need our package to be on nixpkgs or at least inside a nixpkgs fork and our package emits 2 binaries whilst nix-bundle seems to only emit one. This might be fixable if we split installer and winapps itself into two nix packages though

@oskardotglobal
Copy link
Member Author

I think this PR can be merged now though, we can continue this discussion in #234.

@oskardotglobal oskardotglobal merged commit 350f003 into main Sep 7, 2024
2 checks passed
@AkechiShiro
Copy link

AkechiShiro commented Sep 7, 2024

I thought I'd have time to do a retest before the merge, so we can identify potential improvement points and mark them as open issues, that would get resolved in further PRs.

@oskardotglobal
Copy link
Member Author

I did test this extensively, but the bugs you are facing are unrelated to this PR itself; they are general problems with how the installer works.

@AkechiShiro
Copy link

Then we should open issue to track them, because I don't know them by heart on my side and I will forget about them after a few weeks

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.

"Please ensure Windows is powered on" wrong help text
4 participants