API draft for the rewrite

[oskardotglobal]

This PR is a draft for how the API of the rewrite could look like.
There are no implementations yet, but once the API is stable the implementations for features can gradually be put in place.

The main difference between the design of legacy WinApps is that the config is the single source of truth for installed apps and will later also contain paths to the icons for the installed apps, meaning we can get rid of the "pre-configured" apps currently found in the apps folder. A sample config would look like this:

[auth]
username = "Docker"
password = ""
ssh_port = 2222
domain = ""

[container]
enable = true
enable_podman = false
container_name = "WinApps"

[libvirt]
enable = false
vm_name = "RDPWindows"

[manual]
enable = false
host = "127.0.0.1"

[freerdp]
extra_args = [
    "/cert:ignore",
    "+auto-reconnect",
    "+span",
]
executable = "xfreerdp"

[[linked_apps]]
id = "explorer"
name = "Explorer"
win_exec = "explorer.exe"

# [[linked_apps]]
# id = ...

TODOs

• Libvirt implementation
• Basic app detection
• Uninstalling apps
• Changing the RDP port
• Loading apps from multiple files / a well-defined folder / ... so we can better ship pre-defined apps

[oskardotglobal]

I've been working on this and there are still some implementations and features missing, but as far as the API goes I think it's pretty good.

This is currently still missing the entire libvirt implementation as well as the MULTIMON and SCALE config settings and the app detection, which doesn't even have a proper Api yet.

However, the connection check using a TcpStream should already be more stable than the simple check using Netcat, so I think there's a lot of value to be had here once this is more feature-complete.

[LDprg]

This seems great (just a quick look, no proper review). I didn't checked the repo in a while (as written before I sadly lack time to work on winapps at the moment). Didn't expect this to be that far.

[oskardotglobal]

Ok, I've had some more time to work on this.

The libvirt implementation is still missing, but I'm working towards apps now. The most notable change is the addition of a custom docker image, which already includes the OEM files. The install script has also been changed to now set up an SSH server automatically, which I'll be using for discovering the apps.

This means that basically every feature from the winapps script itself is implemented, I just need the installer logic now; so this should be ready for review soon.

[oskardotglobal]

Thanks, git

[oskardotglobal]

Right, rebasing to sign off commits broke history, so I had to scrap all of it, forcing me to squash everything into one commit. Sorry about that.

[oskardotglobal]

After almost a year, I think this PR is in a state where it can be tested. Libvirt is not implemented yet and apps can only be installed, not uninstalled for now, but the program runs as it's supposed to.

If anyone wants to test this that would be very helpful, I'm glad to provide instructions

[coderabbitai]

Actionable comments posted: 23

♻️ Duplicate comments (2)

shell.nix (1)

1-11: Pin nixpkgs with a commit hash for reproducibility.

The nixpkgs tarball URL lacks a SHA256 hash, making builds non-reproducible and potentially insecure. Additionally, ensure the version tag (nixos-25.05) is appropriate for your target release.

To properly pin nixpkgs, use fetchTarball with url and sha256:

 {
-  nixpkgs ? fetchTarball "https://github.com/NixOS/nixpkgs/archive/nixos-25.05.tar.gz",
+  nixpkgs ? fetchTarball {
+    url = "https://github.com/NixOS/nixpkgs/archive/nixos-25.05.tar.gz";
+    sha256 = "sha256:0000000000000000000000000000000000000000000000000000";  # Replace with actual hash
+  },
   fenix-src ? fetchTarball "https://github.com/nix-community/fenix/archive/monthly.tar.gz",

Run nix-prefetch-url --unpack <url> to get the correct SHA256 hash. Consider also pinning fenix-src with a hash. As per past review comments.

winapps/src/backend/container.rs (1)

27-28: Return a handled config error instead of panicking.

assert!(config.container.enable); will abort the process on misconfiguration. Replace it with ensure! (or equivalent) so we surface Error::Config rather than crashing.

-        assert!(config.container.enable);
+        ensure!(
+            config.container.enable,
+            Error::Config("Container backend is disabled")
+        );

📜 Review details

Configuration used: CodeRabbit UI

Review profile: ASSERTIVE

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 0fd9a8c and a3781a7.

⛔ Files ignored due to path filters (1)

• Cargo.lock is excluded by !**/*.lock

📒 Files selected for processing (48)

• .envrc.example (1 hunks)
• .github/CODE_OF_CONDUCT.md (1 hunks)
• .github/CONTRIBUTING.md (1 hunks)
• .github/ISSUE_TEMPLATE/1-bug.yml (1 hunks)
• .github/ISSUE_TEMPLATE/config.yml (1 hunks)
• .github/workflows/build.yml (0 hunks)
• .github/workflows/ci.yaml (1 hunks)
• .github/workflows/docker.yaml (1 hunks)
• .github/workflows/rust-clippy.yml (0 hunks)
• .github/workflows/rust.yml (0 hunks)
• .gitignore (1 hunks)
• .pre-commit-config.yaml (2 hunks)
• .rustfmt.toml (1 hunks)
• CONTRIBUTING.md (0 hunks)
• Cargo.toml (0 hunks)
• LICENSE (0 hunks)
• LICENSE.md (1 hunks)
• README.md (1 hunks)
• compose.yaml (1 hunks)
• scripts/install.sh (0 hunks)
• scripts/install_quickemu.py (0 hunks)
• shell.nix (1 hunks)
• winapps-cli/Cargo.toml (1 hunks)
• winapps-cli/src/main.rs (1 hunks)
• winapps-gui/Cargo.toml (0 hunks)
• winapps-gui/src/main.rs (0 hunks)
• winapps-image/Containerfile (1 hunks)
• winapps-image/oem/Container.reg (1 hunks)
• winapps-image/oem/ExtractPrograms.ps1 (1 hunks)
• winapps-image/oem/NetProfileCleanup.ps1 (1 hunks)
• winapps-image/oem/RDPApps.reg (1 hunks)
• winapps-image/oem/install.bat (1 hunks)
• winapps-image/oem/install.ps1 (1 hunks)
• winapps/Cargo.toml (1 hunks)
• winapps/src/backend/container.rs (1 hunks)
• winapps/src/backend/manual.rs (1 hunks)
• winapps/src/backend/mod.rs (1 hunks)
• winapps/src/command.rs (1 hunks)
• winapps/src/config/apps.rs (1 hunks)
• winapps/src/config/mod.rs (1 hunks)
• winapps/src/config/operations.rs (1 hunks)
• winapps/src/dirs.rs (1 hunks)
• winapps/src/errors.rs (1 hunks)
• winapps/src/freerdp.rs (0 hunks)
• winapps/src/lib.rs (1 hunks)
• winapps/src/quickemu.rs (0 hunks)
• winapps/src/remote_client/freerdp.rs (1 hunks)
• winapps/src/remote_client/mod.rs (1 hunks)

💤 Files with no reviewable changes (12)

• Cargo.toml
• .github/workflows/rust-clippy.yml
• winapps/src/freerdp.rs
• winapps-gui/Cargo.toml
• LICENSE
• scripts/install.sh
• .github/workflows/rust.yml
• winapps-gui/src/main.rs
• .github/workflows/build.yml
• winapps/src/quickemu.rs
• CONTRIBUTING.md
• scripts/install_quickemu.py

🧰 Additional context used

🪛 actionlint (1.7.7)

.github/workflows/docker.yaml

29-29: the runner of "docker/metadata-action@v4" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

🪛 markdownlint-cli2 (0.18.1)

.github/CODE_OF_CONDUCT.md

64-64: Bare URL used

(MD034, no-bare-urls)

.github/CONTRIBUTING.md

5-5: Bare URL used

(MD034, no-bare-urls)

---

6-6: Bare URL used

(MD034, no-bare-urls)

🪛 YAMLlint (1.37.1)

.github/workflows/docker.yaml

[warning] 3-3: truthy value should be one of [false, true]

(truthy)

---

[error] 5-5: too many spaces inside brackets

(brackets)

---

[error] 5-5: too many spaces inside brackets

(brackets)

---

[error] 6-6: too many spaces inside brackets

(brackets)

---

[error] 6-6: too many spaces inside brackets

(brackets)

.github/workflows/ci.yaml

[warning] 3-3: truthy value should be one of [false, true]

(truthy)

---

[error] 5-5: too many spaces inside brackets

(brackets)

---

[error] 5-5: too many spaces inside brackets

(brackets)

---

[error] 7-7: too many spaces inside brackets

(brackets)

---

[error] 7-7: too many spaces inside brackets

(brackets)

🔇 Additional comments (15)

.github/ISSUE_TEMPLATE/config.yml (1)

1-5: LGTM!

The issue template configuration appropriately disables blank issues and directs users to Discussions for community support. The configuration is clear and follows GitHub's best practices.

winapps-image/oem/install.bat (1)

1-3: LGTM!

The batch file correctly delegates to PowerShell with -ExecutionPolicy Bypass, which is appropriate for OEM installation scenarios where script signing may not be available. The use of %~dp0 ensures the script references the correct directory regardless of where it's invoked from.

winapps/Cargo.toml (1)

8-20: LGTM!

The dependency additions and updates are well-chosen:

• parking_lot (0.12.5) provides efficient synchronization primitives
• miette (7.2.0) and thiserror (2.0.9) improve error handling and diagnostics
• dirs (6.0.0), base64 (0.22.1), and regex (1.11.2) add essential utilities
• serde and toml versions are current and stable

All versions align with the latest stable releases and the crates are well-maintained. Based on learnings.

winapps/src/dirs.rs (1)

43-61: Verify the directory structure is intentional.

The directory structure places icons and desktop files in standard XDG locations:

• icons_dir() → ~/.local/share/icons (not ~/.local/share/winapps/icons)
• desktop_dir() → ~/.local/share/applications (not ~/.local/share/winapps/applications)

While this follows XDG standards for system-wide icon and application discovery, it differs from the pattern used by data_dir() and config_dir() which create winapps subdirectories.

Confirm this is intentional. If icons and desktop files should be scoped to winapps:

 pub fn icons_dir() -> Result<PathBuf> {
-    let Some(data_dir) = dirs::data_dir().map(|path| path.join("icons")) else {
+    let Some(data_dir) = dirs::data_dir().map(|path| path.join("winapps").join("icons")) else {
         bail!("Could not determine $XDG_DATA_HOME")
     };
 
 pub fn desktop_dir() -> Result<PathBuf> {
-    let Some(data_dir) = dirs::data_dir().map(|path| path.join("applications")) else {
+    let Some(data_dir) = dirs::data_dir().map(|path| path.join("winapps").join("applications")) else {
         bail!("Could not determine $XDG_DATA_HOME")
     };

Using standard XDG locations allows system-wide discovery but may cause conflicts with other applications. Using winapps subdirectories isolates the app data but requires additional desktop environment configuration for discovery.

winapps-image/oem/ExtractPrograms.ps1 (3)

6-49: LGTM! Resource cleanup is thorough.

The icon extraction logic correctly handles errors with a fallback PNG, and all GDI+ resources are properly disposed.

---

371-377: LGTM! Proper conditional execution.

The sequential logic correctly checks for Get-AppxPackage cmdlet availability before calling UWP search, which prevents errors on systems without UWP support.

---

348-348: Consider adding validation for regex match.

While the regex pattern looks correct, the code immediately uses $matches[1] after a successful match. If the pattern is ever modified incorrectly, this could throw an error.

Consider adding explicit validation:

                if ($line -match '^\s*path\s*=\s*"([^"]+)"')
                {
-                    $exePath = $matches[1]
+                    if ($matches.Count -ge 2) {
+                        $exePath = $matches[1]
+                    }
                    break
                }

Likely an incorrect or invalid review comment.

winapps-image/Containerfile (1)

1-3: LGTM! Clean container image definition.

The Containerfile correctly uses the dockur/windows base image and copies the OEM customization files into the image.

compose.yaml (2)

17-17: Verify HOME environment variable usage.

The compose file passes through the HOME environment variable from the host. Ensure this is intentional and that the Windows container can properly interpret this Linux/macOS path.

Consider documenting the expected behavior or using a more explicit configuration value if the container requires specific path handling.

---

29-31: Verify KVM and TUN device requirements.

The container requires access to /dev/kvm and /dev/net/tun devices. Ensure that:

1. The host system has KVM virtualization enabled
2. The user running docker has permissions to access these devices
3. Documentation mentions these requirements for users

These are typical requirements for running nested virtualization, but should be clearly documented in setup instructions.

winapps-image/oem/RDPApps.reg (1)

3-20: Verify registry file indentation and empty key creation.

Two concerns:

1. Indentation: Similar to Container.reg, the registry keys have leading whitespace. This is non-standard and may cause import failures.

2. Empty key at line 20: The NewNetworkWindowOff key is defined without a value. If the intent is to create the key (not set a value), this is correct. However, if a value should be set, it's missing.

Verify both the indentation pattern and whether line 20 should have a value assigned.

.github/workflows/docker.yaml (2)

33-50: LGTM! Build and push configuration is correct.

The workflow correctly:

• Builds using buildah with proper context and containerfile
• Extracts metadata for tagging
• Pushes to GHCR with GitHub token authentication

---

3-10: Fix YAML formatting issues.

Static analysis identified several formatting issues:

1. Line 3: on should not have quotes around boolean values in YAML
2. Lines 5-6: Excessive spacing inside brackets

Apply this diff to fix the formatting:

 on:
   push:
-    branches: [ "rewrite" ]
-    paths: [ "winapps-image/**" ]
+    branches: ["rewrite"]
+    paths: ["winapps-image/**"]
   workflow_dispatch:
   schedule:

Likely an incorrect or invalid review comment.

winapps/src/remote_client/mod.rs (1)

1-11: LGTM! Clean trait definition.

The RemoteClient trait provides a well-defined interface for remote client backends with appropriate method signatures and error handling.

winapps-image/oem/Container.reg (1)

3-4: No action needed for registry file indentation.

Indentation is consistent across all .reg files and leading spaces are ignored by Windows registry import.

Likely an incorrect or invalid review comment.

[LDprg]

Great work @oskardotglobal!
I made a quick review for now (I think we will need to do this in multiply cycles because of the PR size).

I don't know what I should think of those AI reviews. I dislike the thought of AI companies making money of open source projects without asking the developers/maintainers.

[LDprg]

For precommit we could possibly add renovate bot it could check the dependencies (also for cargo) and make automatic PRs. I not completely sure if this works with out DCO, but just a thought, since it would help a bit to keep this updated.

[oskardotglobal]

I really dislike renovate because you get a gajillion random PRs and it makes it look like a project is very active when in fact all the commits in the last three years were made by renovate. However, the idea itself is good; keeping our dependencies up to date wouldn't hurt.

[LDprg]

I think renovate could be configured to only give you one PR for all updates:
However, we could also use another bot or a CI/CD for this, if you wish so.

[LDprg]

I am not a fan of this. Doesn't it makes more sense to wrap the config in a Rc or Arc? Referencing a RwLock feels very much bad habit. I guess we do not have the overhead of a Rc, but I don't think thats important in our case and RwLock does have a big overhead already.

I feel like we need a better solution to this.

[oskardotglobal]

Yeah, this is why I wanted someone experienced to review this. The RwLock is here because we do actually need to write to the config sometimes, like when installing apps.

I'm open to ideas, but I don't think an Arc/Rc has interior mutability, does it?

[LDprg]

So the way this works is that Arc/Rc are similar to C++ smart pointers, but with immutability.
If you need mutability you can simply use Arc<Mutex<>> for multi threaded programs and Rc<RefCell<>> for single threaded ones.
Both are much cheaper in overhead than RwLock (while Rc by far the one with the least overhead). The thing about RwLock is that it provides multiple read access at the same time in multi threaded use cases (and only one write access at the time). While Arc/Mutex provides only one read and one write (Rc is single threaded anyways so no accesses at the same time possible)

As far as I see Rc<RefCell<>> should be enough for us.

[LDprg]

I think lazy_static would be a better solution. I really dislike OnceLock for its syntax and usability.

[oskardotglobal]

Yes, that would be good. However, at the moment this is not possible since the init operation is fallible, and there's no good way to propagate the error.

[LDprg]

Couldn't we just move the init code to its own function that could give a Result and do the error handling within the lazy_static?
If I am not mistaken, we don't need to propagate the error since if the code fails we won't resume execution anyways. So could we not just print and exit?

[oskardotglobal]

How do you want to handle the error within a lazy_static? The only option we'd have is just panicking

[LDprg]

You could manually catch the error and then print it to stderr/stdin and exit without panicking within a function that generates the value of the lazy_static variable. We won't need to forward the error to main since it panics there anyways and would simply print and exit the program.
Of course a simple panic would also enough, but we could close the app controlled in this case (which is what happens like you implemented anyways).

Edit: There are also LazyLocks.

[LDprg]

We should update the packages to latest and switch from edition="2021" to edition="2024".
Also switching to rust workspaces would help alot.

[oskardotglobal]

I've addressed some of your comments. Regarding the AI review, I triggered that myself; I wanted to try it. I honestly can't argue against the quality; it was pretty good. There are settings which auto-generate PR summaries and auto-review PRs which I find to be too intrusive, but if triggered manually I think it could be useful. We should have a discussion about an AI policy for our project, though.