Add QWT support #181

omeg · 2025-02-10T16:24:31Z

No description provided.

marmarek

This isn't a full review yet, but I think I captured main point here already.

marmarek · 2025-02-18T15:25:32Z

rpc/qubes.WinSign.CreateKey

+set -efo pipefail
+
+# shellcheck source=SCRIPTDIR/qubes.WinSign.common
+. /etc/qubes-rpc/qubes.WinSign.common


Better not hardcode the path here. Especially, it's convenient to install the RPC files in /usr/local/etc/qubes-rpc in a specific AppVM.
You can use $(dirname "$0") to find the directory.

Similar comment to all the other services too.

marmarek · 2025-02-18T15:26:15Z

tools/windows/edit-iso.sh

+
+echo "[*] Copying the final iso from '${DISPVM}' to '${OUTPUT}'..."
+
+shell_call "${DISPVM}" "cat ~/win-build.iso" | cat > "${OUTPUT}"


Why extra cat?

Suggested change

shell_call "${DISPVM}" "cat ~/win-build.iso" | cat > "${OUTPUT}"

shell_call "${DISPVM}" "cat ~/win-build.iso" > "${OUTPUT}"

marmarek · 2025-02-18T15:28:46Z

qubesbuilder/plugins/build_windows/__init__.py

+                    key_name=sign_key_name,
+                )
+
+            dvm.kill()


If starting dvm fails, this will fail too (UnboundLocalError: cannot access local variable 'dvm' where it is not associated with a value). Either move starting dvm before the try part, or initialize the variable early (with None?) and check for it here.
But also, it looks like the dvm start can be moved much later? Currently it's at the start of the build, but it's used only after the build - so, if you move it later, you can save starting it in case of build failure.

marmarek · 2025-02-18T15:29:06Z

rpc/policy/51-qubesbuilder-windows.policy

+qubes.WinSign.CreateKey  +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.DeleteKey  +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.GetCert    +Qubes__Windows__Tools  work-qubesos  vault-sign  allow
+qubes.WinSign.Sign       +Qubes__Windows__Tools  work-qubesos  vault-sign  allow


Missing policy for the timestamp service

marmarek · 2025-02-18T15:30:11Z

rpc/policy/51-qubesbuilder-windows.policy

+admin.vm.property.Get            *  work-qubesos  win-build     allow  target=dom0
+admin.vm.device.block.Attach     *  work-qubesos  win-build     allow  target=dom0
+
+qubes.WinSign.QueryKey   +Qubes__Windows__Tools  work-qubesos  vault-sign  allow


Generally for builder-specific services we use qubesbuilder. prefix.

marmarek · 2025-02-18T15:50:57Z

qubesbuilder/executors/windows.py

+    def run(
+        self,
+        cmd: List[str],
+        copy_in: List[Tuple[Path, PurePath]] = None,
+        copy_out: List[Tuple[PurePath, Path]] = None,
+    ):


To support building in DispVM, this should be able to create and start new DispVM first and kill it at the end.
It does mean fresh DispVM for each component - is that a problem? In other words - are there some hidden assumption about reusing the same VM (like, depending on some files being present from previous build)?

For development builds it probably make sense to retain option to reuse the same VM over and over, so the DispVM case can be under some config option.

I think it can be a DispVM all the time, for development I usually just do local builds without the builder.

marmarek · 2025-02-18T15:58:00Z

qubesbuilder/executors/windows.py

+from qubesadmin import Qubes
+from qubesadmin.devices import DeviceAssignment, UnknownDevice
+from qubesadmin.exc import DeviceAlreadyAttached, QubesException
+from qubesadmin.utils import encode_for_vmexec
+from qubesadmin.vm import DispVM, QubesVM


It should be possible to use qubes-builderv2 outside of Qubes VM too. It doesn't need to support building Windows code there, but it shouldn't crash on ImportError. I think there need to be try / except ImportError somewhere, that will catch it nicely. I have an idea where - see separate comment.

marmarek · 2025-02-18T15:58:51Z

qubesbuilder/config.py

@@ -31,6 +31,7 @@
 from qubesbuilder.executors.container import ContainerExecutor
 from qubesbuilder.executors.local import LocalExecutor
 from qubesbuilder.executors.qubes import LinuxQubesExecutor
+from qubesbuilder.executors.windows import WindowsExecutor


You can catch ImportError here, and set some variable like windows_executor_available = False

marmarek · 2025-02-18T15:59:18Z

qubesbuilder/config.py

@@ -575,6 +576,8 @@ def get_executor(options):
            executor = LocalExecutor(**executor_options)  # type: ignore
        elif executor_type == "qubes":
            executor = LinuxQubesExecutor(**executor_options)  # type: ignore
+        elif executor_type == "windows":
+            executor = WindowsExecutor(**executor_options)  # type: ignore


And here throw an exception if windows_executor_available is false.

marmarek · 2025-02-18T16:05:31Z

qubesbuilder/plugins/build_windows/__init__.py

+from qubesbuilder.config import Config
+from qubesbuilder.distribution import QubesDistribution
+from qubesbuilder.executors import ExecutorError
+from qubesbuilder.executors.windows import WindowsExecutor


And similarly this (and the qubesadmin imports above) can be problematic. It wants either some wrapping here (and raise an exception early in run if qubesadmin is not available), or changing PluginManager so that plugin import error is not fatal. IMO the first option is better if wouldn't clutter the code too much.

FWIW several CI tests that are unrelated to QWT fail due to importing qubesadmin. That could be a good check if it works as expected - tests not related to QWT should pass even if qubesadmin cannot be imported.

I removed the qubesadmin dependency.

Signed-off-by: Rafał Wojdyła <[email protected]>

marmarek · 2025-02-26T17:57:09Z

It has conflicts now... @fepitre in the meantime added most of the cross-distribution dependencies that will be needed for building iso+rpm (remaining part is in #185, but that shouldn't conflict anymore, I hope).

Signed-off-by: Rafał Wojdyła <[email protected]>

Add option to not ask user to enable network discovery Signed-off-by: Rafał Wojdyła <[email protected]>

Signed-off-by: Rafał Wojdyła <[email protected]>

…age options Signed-off-by: Rafał Wojdyła <[email protected]>

Signed-off-by: Rafał Wojdyła <[email protected]>

codecov · 2025-02-27T14:46:32Z

Codecov Report

Attention: Patch coverage is 36.34497% with 310 lines in your changes missing coverage. Please review.

Project coverage is 75.12%. Comparing base (1987baa) to head (5f3210a).

Files with missing lines	Patch %	Lines
qubesbuilder/plugins/build_windows/__init__.py	21.93%	153 Missing ⚠️
qubesbuilder/executors/qubes.py	31.62%	80 Missing ⚠️
qubesbuilder/executors/windows.py	37.50%	35 Missing ⚠️
qubesbuilder/plugins/source_windows/__init__.py	42.10%	22 Missing ⚠️
qubesbuilder/plugins/__init__.py	46.15%	7 Missing ⚠️
qubesbuilder/executors/qrexec.py	89.58%	5 Missing ⚠️
qubesbuilder/config.py	33.33%	4 Missing ⚠️
qubesbuilder/distribution.py	60.00%	4 Missing ⚠️

Additional details and impacted files

@@            Coverage Diff             @@
##             main     #181      +/-   ##
==========================================
- Coverage   78.74%   75.12%   -3.63%     
==========================================
  Files          47       51       +4     
  Lines        5388     5825     +437     
==========================================
+ Hits         4243     4376     +133     
- Misses       1145     1449     +304

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

marmarek · 2025-02-27T14:49:57Z

Can you squash the fixup commits (just do git rebase -i --autosquash)

Signed-off-by: Rafał Wojdyła <[email protected]>

…utor Signed-off-by: Rafał Wojdyła <[email protected]>

Signed-off-by: Rafał Wojdyła <[email protected]>

tlaurion · 2025-02-28T20:45:30Z

Amazing work @omeg ! Can't wait to test testing packages. Also really interesting piece of work in terms of complex qubes-builderv2 for a learner. Thanks!

marmarek requested changes Feb 18, 2025

View reviewed changes

Add support for Windows executor and build

12d3f67

Signed-off-by: Rafał Wojdyła <[email protected]>

omeg added 15 commits February 26, 2025 18:57

windows: add support for code signing

9bdf7c6

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: improve unattended OS installation

b964409

Add option to not ask user to enable network discovery Signed-off-by: Rafał Wojdyła <[email protected]>

Fix typos in README

b091107

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix shellcheck warnings in scripts

212940e

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix download script call

b6501c3

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: untabify autounattend.xml

37bbc89

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: generate random user password during worker VM setup

8390740

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix existing file check in get-files.sh

5bad846

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix missing configuration option in README

5ac9452

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: move the CONFIGURATION placeholder from executor to build st…

d8aab4e

…age options Signed-off-by: Rafał Wojdyła <[email protected]>

windows: automatically create loop device for EWDK

298fe55

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: edit the installation iso in a dispvm

0d05f14

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: update RPC policy files

c8b9987

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: refactor signing

10f4ac5

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: timestamp when code signing

12fe446

Signed-off-by: Rafał Wojdyła <[email protected]>

omeg force-pushed the omeg/windows branch from 3ceadd4 to a23caaf Compare February 26, 2025 19:45

omeg added 9 commits February 27, 2025 15:56

windows: support build via qrexec services

2f46e61

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: add scripts for local build

83729a5

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: remove redundant code

db70cbf

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: add note about MS Defender in the readme

eb90a85

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: remove unneeded checks of vm status to improve performance

84e4f85

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: reword 'mount' to 'attach' for EWDK loopback device

26e4737

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: create local .distfiles only if necessary

a5ccf6f

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix testsign switch in the build plugin

b965b0f

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: improve copy_out performance

382d11b

Signed-off-by: Rafał Wojdyła <[email protected]>

omeg added 15 commits February 27, 2025 15:57

windows: fix local build

10abbea

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix local build compatibility in build_windows

7713060

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: don't copy qrexec handlers on every command

03a9289

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix copy-out for no-target projects

a52949c

Signed-off-by: Rafał Wojdyła <[email protected]>

Fix formatting

c08a8bf

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: remove debug code

7eef505

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix mypy warnings

c25d8f5

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: update CI dependencies

d74a23f

Signed-off-by: Rafał Wojdyła <[email protected]>

Fix CI config

d9a809e

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix shellcheck warnings in rpc files

977fe2b

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: fix review issues

d9984d9

Signed-off-by: Rafał Wojdyła <[email protected]>

Refactor WindowsExecutor into WindowsQubesExecutor and SSHWindowsExec…

459a5d9

…utor Signed-off-by: Rafał Wojdyła <[email protected]>

windows: sanitize_line() compatibility

819b714

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: remove qubesadmin dependency

279996b

Signed-off-by: Rafał Wojdyła <[email protected]>

windows: update README

5f3210a

Signed-off-by: Rafał Wojdyła <[email protected]>

omeg force-pushed the omeg/windows branch from 40e0082 to 5f3210a Compare February 27, 2025 15:26

omeg marked this pull request as ready for review February 27, 2025 15:36

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add QWT support #181

Add QWT support #181

omeg commented Feb 10, 2025

marmarek left a comment

marmarek Feb 18, 2025

omeg Feb 20, 2025

marmarek Feb 18, 2025

omeg Feb 20, 2025

marmarek Feb 18, 2025

omeg Feb 26, 2025

marmarek Feb 18, 2025

omeg Feb 20, 2025

marmarek Feb 18, 2025

omeg Feb 20, 2025

marmarek Feb 18, 2025

omeg Feb 20, 2025

marmarek Feb 18, 2025

marmarek Feb 18, 2025

marmarek Feb 18, 2025

marmarek Feb 18, 2025

marmarek Feb 21, 2025

omeg Feb 27, 2025

marmarek commented Feb 26, 2025

codecov bot commented Feb 27, 2025 •

edited

Loading

marmarek commented Feb 27, 2025

tlaurion commented Feb 28, 2025 •

edited

Loading


		echo "[*] Copying the final iso from '${DISPVM}' to '${OUTPUT}'..."

		shell_call "${DISPVM}" "cat ~/win-build.iso" \| cat > "${OUTPUT}"

Add QWT support #181

Are you sure you want to change the base?

Add QWT support #181

Conversation

omeg commented Feb 10, 2025

marmarek left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marmarek commented Feb 26, 2025

codecov bot commented Feb 27, 2025 • edited Loading

Codecov Report

marmarek commented Feb 27, 2025

tlaurion commented Feb 28, 2025 • edited Loading

codecov bot commented Feb 27, 2025 •

edited

Loading

tlaurion commented Feb 28, 2025 •

edited

Loading