[PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Bin Meng]

At present packaging the required DLLs of QEMU executables is a
manual process, and error prone.

Improve scripts/nsis.py by adding a logic to automatically package
required DLLs of QEMU executables.

'make installer' is tested in the cross-build on Linux in CI, but
not in the Windows native build. Update CI to test the installer
generation on Windows too.

During testing a 32-bit build issue was exposed in block/nfs.c and
the fix is included in this series.


Bin Meng (7):
  scripts/nsis.py: Drop the unnecessary path separator
  scripts/nsis.py: Fix destination directory name when invoked on
    Windows
  scripts/nsis.py: Automatically package required DLLs of QEMU
    executables
  .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
  block/nfs: Fix 32-bit Windows build
  .gitlab-ci.d/windows.yml: Unify the prerequisite packages
  .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

 meson.build              |  1 +
 block/nfs.c              |  8 ++++++
 .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
 scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
 4 files changed, 89 insertions(+), 20 deletions(-)

-- 
2.34.1

[PATCH 1/7] scripts/nsis.py: Drop the unnecessary path separator

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

There is no need to append a path separator to the destination
directory that is passed to "make install".

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 scripts/nsis.py | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/scripts/nsis.py b/scripts/nsis.py
index 462d6cac3b..bbb41d9386 100644
--- a/scripts/nsis.py
+++ b/scripts/nsis.py
@@ -30,7 +30,7 @@ def main():
 
     destdir = tempfile.mkdtemp()
     try:
-        subprocess.run(["make", "install", "DESTDIR=" + destdir + os.path.sep])
+        subprocess.run(["make", "install", "DESTDIR=" + destdir])
         with open(
             os.path.join(destdir + args.prefix, "system-emulations.nsh"), "w"
         ) as nsh, open(
-- 
2.34.1

[PATCH 2/7] scripts/nsis.py: Fix destination directory name when invoked on Windows

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

"make installer" on Windows fails with the following message:

  Traceback (most recent call last):
    File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 89, in <module>
      main()
    File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 34, in main
      with open(
  OSError: [Errno 22] Invalid argument:
  'R:/Temp/tmpw83xhjquG:/msys64/qemu/system-emulations.nsh'
  ninja: build stopped: subcommand failed.

Use os.path.splitdrive() to form a canonical path without the drive
letter on Windows. This works with cross-build on Linux too.

Fixes: 8adfeba953e0 ("meson: add NSIS building")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 scripts/nsis.py | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/scripts/nsis.py b/scripts/nsis.py
index bbb41d9386..baa6ef9594 100644
--- a/scripts/nsis.py
+++ b/scripts/nsis.py
@@ -28,16 +28,18 @@ def main():
     parser.add_argument("nsisargs", nargs="*")
     args = parser.parse_args()
 
+    # canonicalize the Windows native prefix path
+    prefix = os.path.splitdrive(args.prefix)[1]
     destdir = tempfile.mkdtemp()
     try:
         subprocess.run(["make", "install", "DESTDIR=" + destdir])
         with open(
-            os.path.join(destdir + args.prefix, "system-emulations.nsh"), "w"
+            os.path.join(destdir + prefix, "system-emulations.nsh"), "w"
         ) as nsh, open(
-            os.path.join(destdir + args.prefix, "system-mui-text.nsh"), "w"
+            os.path.join(destdir + prefix, "system-mui-text.nsh"), "w"
         ) as muinsh:
             for exe in sorted(glob.glob(
-                os.path.join(destdir + args.prefix, "qemu-system-*.exe")
+                os.path.join(destdir + prefix, "qemu-system-*.exe")
             )):
                 exe = os.path.basename(exe)
                 arch = exe[12:-4]
@@ -61,7 +63,7 @@ def main():
                 !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
                 """.format(arch, desc))
 
-        for exe in glob.glob(os.path.join(destdir + args.prefix, "*.exe")):
+        for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
             signcode(exe)
 
         makensis = [
@@ -69,7 +71,7 @@ def main():
             "-V2",
             "-NOCD",
             "-DSRCDIR=" + args.srcdir,
-            "-DBINDIR=" + destdir + args.prefix,
+            "-DBINDIR=" + destdir + prefix,
         ]
         dlldir = "w32"
         if args.cpu == "x86_64":
-- 
2.34.1

[PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present packaging the required DLLs of QEMU executables is a
manual process, and error prone.

Actually build/config-host.mak contains a GLIB_BINDIR variable
which is the directory where glib and other DLLs reside. This
works for both Windows native build and cross-build on Linux.
We can use it as the search directory for DLLs and automate
the whole DLL packaging process.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 meson.build     |  1 +
 scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
 2 files changed, 43 insertions(+), 4 deletions(-)

diff --git a/meson.build b/meson.build
index c2adb7caf4..4c03850f9f 100644
--- a/meson.build
+++ b/meson.build
@@ -3657,6 +3657,7 @@ if host_machine.system() == 'windows'
     '@OUTPUT@',
     get_option('prefix'),
     meson.current_source_dir(),
+    config_host['GLIB_BINDIR'],
     host_machine.cpu(),
     '--',
     '-DDISPLAYVERSION=' + meson.project_version(),
diff --git a/scripts/nsis.py b/scripts/nsis.py
index baa6ef9594..03ed7608a2 100644
--- a/scripts/nsis.py
+++ b/scripts/nsis.py
@@ -18,12 +18,36 @@ def signcode(path):
         return
     subprocess.run([cmd, path])
 
+def find_deps(exe_or_dll, search_path, analyzed_deps):
+    deps = [exe_or_dll]
+    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
+    output = output.split("\n")
+    for line in output:
+        if not line.startswith("\tDLL Name: "):
+            continue
+
+        dep = line.split("DLL Name: ")[1].strip()
+        if dep in analyzed_deps:
+            continue
+
+        dll = os.path.join(search_path, dep)
+        if not os.path.exists(dll):
+            # assume it's a Windows provided dll, skip it
+            continue
+
+        analyzed_deps.add(dep)
+        # locate the dll dependencies recursively
+        rdeps = find_deps(dll, search_path, analyzed_deps)
+        deps.extend(rdeps)
+
+    return deps
 
 def main():
     parser = argparse.ArgumentParser(description="QEMU NSIS build helper.")
     parser.add_argument("outfile")
     parser.add_argument("prefix")
     parser.add_argument("srcdir")
+    parser.add_argument("dlldir")
     parser.add_argument("cpu")
     parser.add_argument("nsisargs", nargs="*")
     args = parser.parse_args()
@@ -63,9 +87,26 @@ def main():
                 !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
                 """.format(arch, desc))
 
+        search_path = args.dlldir
+        print("Searching '%s' for the dependent dlls ..." % search_path)
+        dlldir = os.path.join(destdir + prefix, "dll")
+        os.mkdir(dlldir)
+
         for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
             signcode(exe)
 
+            # find all dll dependencies
+            deps = set(find_deps(exe, search_path, set()))
+            deps.remove(exe)
+
+            # copy all dlls to the DLLDIR
+            for dep in deps:
+                dllfile = os.path.join(dlldir, os.path.basename(dep))
+                if (os.path.exists(dllfile)):
+                    continue
+                print("Copying '%s' to '%s'" % (dep, dllfile))
+                shutil.copy(dep, dllfile)
+
         makensis = [
             "makensis",
             "-V2",
@@ -73,12 +114,9 @@ def main():
             "-DSRCDIR=" + args.srcdir,
             "-DBINDIR=" + destdir + prefix,
         ]
-        dlldir = "w32"
         if args.cpu == "x86_64":
-            dlldir = "w64"
             makensis += ["-DW64"]
-        if os.path.exists(os.path.join(args.srcdir, "dll")):
-            makensis += ["-DDLLDIR={0}/dll/{1}".format(args.srcdir, dlldir)]
+        makensis += ["-DDLLDIR=" + dlldir]
 
         makensis += ["-DOUTFILE=" + args.outfile] + args.nsisargs
         subprocess.run(makensis)
-- 
2.34.1

[PATCH 4/7] .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

The sed processing of build/config-host.mak seems to be no longer
needed, and there is no such in the 32-bit build too. Drop it.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 .gitlab-ci.d/windows.yml | 1 -
 1 file changed, 1 deletion(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index da6013904a..86a4339c48 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -60,7 +60,6 @@ msys2-64bit:
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
   - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
       --enable-capstone --without-default-devices'
-  - .\msys64\usr\bin\bash -lc "sed -i '/^ROMS=/d' build/config-host.mak"
   - .\msys64\usr\bin\bash -lc 'make'
   - .\msys64\usr\bin\bash -lc 'make check'
 
-- 
2.34.1

[PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

libnfs.h declares nfs_fstat() as the following for win32:

  int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
                struct __stat64 *st);

The 'st' parameter should be of type 'struct __stat64'. The
codes happen to build successfully for 64-bit Windows, but it
does not build for 32-bit Windows.

Fixes: 6542aa9c75bc ("block: add native support for NFS")
Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for read-only files")
Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 block/nfs.c | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/block/nfs.c b/block/nfs.c
index 444c40b458..d5d67937dd 100644
--- a/block/nfs.c
+++ b/block/nfs.c
@@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
                                int flags, int open_flags, Error **errp)
 {
     int64_t ret = -EINVAL;
+#ifdef _WIN32
+    struct __stat64 st;
+#else
     struct stat st;
+#endif
     char *file = NULL, *strp = NULL;
 
     qemu_mutex_init(&client->mutex);
@@ -781,7 +785,11 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
                               BlockReopenQueue *queue, Error **errp)
 {
     NFSClient *client = state->bs->opaque;
+#ifdef _WIN32
+    struct __stat64 st;
+#else
     struct stat st;
+#endif
     int ret = 0;
 
     if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {
-- 
2.34.1

[PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

At present the prerequisite packages for 64-bit and 32-bit builds
are slightly different. Let's use the same packages for both.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 .gitlab-ci.d/windows.yml | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index 86a4339c48..fffb202658 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -40,11 +40,15 @@ msys2-64bit:
       mingw-w64-x86_64-gcc
       mingw-w64-x86_64-glib2
       mingw-w64-x86_64-gnutls
+      mingw-w64-x86_64-gtk3
+      mingw-w64-x86_64-libgcrypt
+      mingw-w64-x86_64-libjpeg-turbo
       mingw-w64-x86_64-libnfs
       mingw-w64-x86_64-libpng
       mingw-w64-x86_64-libssh
       mingw-w64-x86_64-libtasn1
       mingw-w64-x86_64-libusb
+      mingw-w64-x86_64-lzo2
       mingw-w64-x86_64-nettle
       mingw-w64-x86_64-ninja
       mingw-w64-x86_64-pixman
@@ -77,16 +81,22 @@ msys2-32bit:
       mingw-w64-i686-gtk3
       mingw-w64-i686-libgcrypt
       mingw-w64-i686-libjpeg-turbo
+      mingw-w64-i686-libnfs
+      mingw-w64-i686-libpng
       mingw-w64-i686-libssh
       mingw-w64-i686-libtasn1
       mingw-w64-i686-libusb
       mingw-w64-i686-lzo2
+      mingw-w64-i686-nettle
       mingw-w64-i686-ninja
       mingw-w64-i686-pixman
       mingw-w64-i686-pkgconf
       mingw-w64-i686-python
+      mingw-w64-i686-SDL2
+      mingw-w64-i686-SDL2_image
       mingw-w64-i686-snappy
-      mingw-w64-i686-usbredir "
+      mingw-w64-i686-usbredir
+      mingw-w64-i686-zstd "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
   - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinG environment
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
-- 
2.34.1

[PATCH 7/7] .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

[Bin Meng]

From: Bin Meng <bin.meng@windriver.com>

Now that we have supported packaging DLLs automatically, let's add
the 'make installer' in the CI and publish the generated installer
file as an artifact.

Increase the job timeout to 90 minutes to accommodate to it.

Signed-off-by: Bin Meng <bin.meng@windriver.com>
---

 .gitlab-ci.d/windows.yml | 27 +++++++++++++++++++--------
 1 file changed, 19 insertions(+), 8 deletions(-)

diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
index fffb202658..3a94d40e73 100644
--- a/.gitlab-ci.d/windows.yml
+++ b/.gitlab-ci.d/windows.yml
@@ -10,7 +10,7 @@
       - ${CI_PROJECT_DIR}/msys64/var/cache
   needs: []
   stage: build
-  timeout: 70m
+  timeout: 90m
   before_script:
   - If ( !(Test-Path -Path msys64\var\cache ) ) {
       mkdir msys64\var\cache
@@ -28,6 +28,11 @@
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
   - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
   - taskkill /F /FI "MODULES eq msys-2.0.dll"
+  artifacts:
+    name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
+    expire_in: 7 days
+    paths:
+      - build/qemu-setup*.exe
 
 msys2-64bit:
   extends: .shared_msys2_builder
@@ -51,6 +56,7 @@ msys2-64bit:
       mingw-w64-x86_64-lzo2
       mingw-w64-x86_64-nettle
       mingw-w64-x86_64-ninja
+      mingw-w64-x86_64-nsis
       mingw-w64-x86_64-pixman
       mingw-w64-x86_64-pkgconf
       mingw-w64-x86_64-python
@@ -60,12 +66,15 @@ msys2-64bit:
       mingw-w64-x86_64-usbredir
       mingw-w64-x86_64-zstd "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
-  - $env:MSYSTEM = 'MINGW64'     # Start a 64 bit Mingw environment
+  - $env:MSYSTEM = 'MINGW64'     # Start a 64-bit MinGW environment
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
-  - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
+  - mkdir build
+  - cd build
+  - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
       --enable-capstone --without-default-devices'
-  - .\msys64\usr\bin\bash -lc 'make'
-  - .\msys64\usr\bin\bash -lc 'make check'
+  - ..\msys64\usr\bin\bash -lc 'make'
+  - ..\msys64\usr\bin\bash -lc 'make check'
+  - ..\msys64\usr\bin\bash -lc 'make installer'
 
 msys2-32bit:
   extends: .shared_msys2_builder
@@ -89,6 +98,7 @@ msys2-32bit:
       mingw-w64-i686-lzo2
       mingw-w64-i686-nettle
       mingw-w64-i686-ninja
+      mingw-w64-i686-nsis
       mingw-w64-i686-pixman
       mingw-w64-i686-pkgconf
       mingw-w64-i686-python
@@ -98,10 +108,11 @@ msys2-32bit:
       mingw-w64-i686-usbredir
       mingw-w64-i686-zstd "
   - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
-  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinG environment
+  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinGW environment
   - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
-  - mkdir output
-  - cd output
+  - mkdir build
+  - cd build
   - ..\msys64\usr\bin\bash -lc "../configure --target-list=ppc64-softmmu"
   - ..\msys64\usr\bin\bash -lc 'make'
   - ..\msys64\usr\bin\bash -lc 'make check'
+  - ..\msys64\usr\bin\bash -lc 'make installer'
-- 
2.34.1

Re: [PATCH 2/7] scripts/nsis.py: Fix destination directory name when invoked on Windows

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 2905 bytes --]

On Thu, Sep 8, 2022 at 5:30 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> From: Bin Meng <bin.meng@windriver.com>
>
> "make installer" on Windows fails with the following message:
>
>   Traceback (most recent call last):
>     File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 89, in
> <module>
>       main()
>     File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 34, in main
>       with open(
>   OSError: [Errno 22] Invalid argument:
>   'R:/Temp/tmpw83xhjquG:/msys64/qemu/system-emulations.nsh'
>   ninja: build stopped: subcommand failed.
>
> Use os.path.splitdrive() to form a canonical path without the drive
> letter on Windows. This works with cross-build on Linux too.
>
> Fixes: 8adfeba953e0 ("meson: add NSIS building")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>

Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


> ---
>
>  scripts/nsis.py | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/scripts/nsis.py b/scripts/nsis.py
> index bbb41d9386..baa6ef9594 100644
> --- a/scripts/nsis.py
> +++ b/scripts/nsis.py
> @@ -28,16 +28,18 @@ def main():
>      parser.add_argument("nsisargs", nargs="*")
>      args = parser.parse_args()
>
> +    # canonicalize the Windows native prefix path
> +    prefix = os.path.splitdrive(args.prefix)[1]
>      destdir = tempfile.mkdtemp()
>      try:
>          subprocess.run(["make", "install", "DESTDIR=" + destdir])
>          with open(
> -            os.path.join(destdir + args.prefix, "system-emulations.nsh"),
> "w"
> +            os.path.join(destdir + prefix, "system-emulations.nsh"), "w"
>          ) as nsh, open(
> -            os.path.join(destdir + args.prefix, "system-mui-text.nsh"),
> "w"
> +            os.path.join(destdir + prefix, "system-mui-text.nsh"), "w"
>          ) as muinsh:
>              for exe in sorted(glob.glob(
> -                os.path.join(destdir + args.prefix, "qemu-system-*.exe")
> +                os.path.join(destdir + prefix, "qemu-system-*.exe")
>              )):
>                  exe = os.path.basename(exe)
>                  arch = exe[12:-4]
> @@ -61,7 +63,7 @@ def main():
>                  !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
>                  """.format(arch, desc))
>
> -        for exe in glob.glob(os.path.join(destdir + args.prefix,
> "*.exe")):
> +        for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
>              signcode(exe)
>
>          makensis = [
> @@ -69,7 +71,7 @@ def main():
>              "-V2",
>              "-NOCD",
>              "-DSRCDIR=" + args.srcdir,
> -            "-DBINDIR=" + destdir + args.prefix,
> +            "-DBINDIR=" + destdir + prefix,
>          ]
>          dlldir = "w32"
>          if args.cpu == "x86_64":
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 4296 bytes --]

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 4767 bytes --]

Hi

(adding Stefan Weil in CC, who currently provides Windows installer)

On Thu, Sep 8, 2022 at 5:34 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> From: Bin Meng <bin.meng@windriver.com>
>
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
>
> Actually build/config-host.mak contains a GLIB_BINDIR variable
> which is the directory where glib and other DLLs reside. This
> works for both Windows native build and cross-build on Linux.
> We can use it as the search directory for DLLs and automate
> the whole DLL packaging process.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>

That seems reasonable to me, although packaging dependencies is not just
about linked DLLs.. There are dynamic stuff, executables, data, legal docs
etc etc. I have no clear picture how is everything really packaged in the
installer tbh (I would recommend msys2 qemu installation at this point)

anyhow, for the patch, as far as I am concerned:
Reviewed-by: Marc-André Lureau <marcandre.lureau@redhat.com>


---
>
>  meson.build     |  1 +
>  scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
>  2 files changed, 43 insertions(+), 4 deletions(-)
>
> diff --git a/meson.build b/meson.build
> index c2adb7caf4..4c03850f9f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3657,6 +3657,7 @@ if host_machine.system() == 'windows'
>      '@OUTPUT@',
>      get_option('prefix'),
>      meson.current_source_dir(),
> +    config_host['GLIB_BINDIR'],
>      host_machine.cpu(),
>      '--',
>      '-DDISPLAYVERSION=' + meson.project_version(),
> diff --git a/scripts/nsis.py b/scripts/nsis.py
> index baa6ef9594..03ed7608a2 100644
> --- a/scripts/nsis.py
> +++ b/scripts/nsis.py
> @@ -18,12 +18,36 @@ def signcode(path):
>          return
>      subprocess.run([cmd, path])
>
> +def find_deps(exe_or_dll, search_path, analyzed_deps):
> +    deps = [exe_or_dll]
> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll],
> text=True)
> +    output = output.split("\n")
> +    for line in output:
> +        if not line.startswith("\tDLL Name: "):
> +            continue
> +
> +        dep = line.split("DLL Name: ")[1].strip()
> +        if dep in analyzed_deps:
> +            continue
> +
> +        dll = os.path.join(search_path, dep)
> +        if not os.path.exists(dll):
> +            # assume it's a Windows provided dll, skip it
> +            continue
> +
> +        analyzed_deps.add(dep)
> +        # locate the dll dependencies recursively
> +        rdeps = find_deps(dll, search_path, analyzed_deps)
> +        deps.extend(rdeps)
> +
> +    return deps
>
>  def main():
>      parser = argparse.ArgumentParser(description="QEMU NSIS build
> helper.")
>      parser.add_argument("outfile")
>      parser.add_argument("prefix")
>      parser.add_argument("srcdir")
> +    parser.add_argument("dlldir")
>      parser.add_argument("cpu")
>      parser.add_argument("nsisargs", nargs="*")
>      args = parser.parse_args()
> @@ -63,9 +87,26 @@ def main():
>                  !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
>                  """.format(arch, desc))
>
> +        search_path = args.dlldir
> +        print("Searching '%s' for the dependent dlls ..." % search_path)
> +        dlldir = os.path.join(destdir + prefix, "dll")
> +        os.mkdir(dlldir)
> +
>          for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
>              signcode(exe)
>
> +            # find all dll dependencies
> +            deps = set(find_deps(exe, search_path, set()))
> +            deps.remove(exe)
> +
> +            # copy all dlls to the DLLDIR
> +            for dep in deps:
> +                dllfile = os.path.join(dlldir, os.path.basename(dep))
> +                if (os.path.exists(dllfile)):
> +                    continue
> +                print("Copying '%s' to '%s'" % (dep, dllfile))
> +                shutil.copy(dep, dllfile)
> +
>          makensis = [
>              "makensis",
>              "-V2",
> @@ -73,12 +114,9 @@ def main():
>              "-DSRCDIR=" + args.srcdir,
>              "-DBINDIR=" + destdir + prefix,
>          ]
> -        dlldir = "w32"
>          if args.cpu == "x86_64":
> -            dlldir = "w64"
>              makensis += ["-DW64"]
> -        if os.path.exists(os.path.join(args.srcdir, "dll")):
> -            makensis += ["-DDLLDIR={0}/dll/{1}".format(args.srcdir,
> dlldir)]
> +        makensis += ["-DDLLDIR=" + dlldir]
>
>          makensis += ["-DOUTFILE=" + args.outfile] + args.nsisargs
>          subprocess.run(makensis)
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 6536 bytes --]

Re: [PATCH 4/7] .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build

[Marc-André Lureau]

[-- Attachment #1: Type: text/plain, Size: 1167 bytes --]

Hi

On Thu, Sep 8, 2022 at 5:33 PM Bin Meng <bmeng.cn@gmail.com> wrote:

> From: Bin Meng <bin.meng@windriver.com>
>
> The sed processing of build/config-host.mak seems to be no longer
> needed, and there is no such in the 32-bit build too. Drop it.
>
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>  .gitlab-ci.d/windows.yml | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
> index da6013904a..86a4339c48 100644
> --- a/.gitlab-ci.d/windows.yml
> +++ b/.gitlab-ci.d/windows.yml
> @@ -60,7 +60,6 @@ msys2-64bit:
>    - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
>    - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
>        --enable-capstone --without-default-devices'
> -  - .\msys64\usr\bin\bash -lc "sed -i '/^ROMS=/d' build/config-host.mak"
>

It looks like it is there to remove the ROMS from the make build. No idea
if that still makes sense. Thomas, do you remember?



>    - .\msys64\usr\bin\bash -lc 'make'
>    - .\msys64\usr\bin\bash -lc 'make check'
>
> --
> 2.34.1
>
>
>

-- 
Marc-André Lureau

[-- Attachment #2: Type: text/html, Size: 1972 bytes --]

Re: [PATCH 4/7] .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build

[Thomas Huth]

On 08/09/2022 16.04, Marc-André Lureau wrote:
> Hi
> 
> On Thu, Sep 8, 2022 at 5:33 PM Bin Meng <bmeng.cn@gmail.com 
> <mailto:bmeng.cn@gmail.com>> wrote:
> 
>     From: Bin Meng <bin.meng@windriver.com <mailto:bin.meng@windriver.com>>
> 
>     The sed processing of build/config-host.mak seems to be no longer
>     needed, and there is no such in the 32-bit build too. Drop it.
> 
>     Signed-off-by: Bin Meng <bin.meng@windriver.com
>     <mailto:bin.meng@windriver.com>>
>     ---
> 
>       .gitlab-ci.d/windows.yml | 1 -
>       1 file changed, 1 deletion(-)
> 
>     diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
>     index da6013904a..86a4339c48 100644
>     --- a/.gitlab-ci.d/windows.yml
>     +++ b/.gitlab-ci.d/windows.yml
>     @@ -60,7 +60,6 @@ msys2-64bit:
>         - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
>         - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
>             --enable-capstone --without-default-devices'
>     -  - .\msys64\usr\bin\bash -lc "sed -i '/^ROMS=/d' build/config-host.mak"
> 
> 
> It looks like it is there to remove the ROMS from the make build. No idea if 
> that still makes sense. Thomas, do you remember?

I originally had to add this sed statement since there was a compile error 
otherwise in the ROMS ... if it now works fine without this line, this 
should be fine, of course, too.

Reviewed-by: Thomas Huth <thuth@redhat.com>

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Thomas Huth]

On 08/09/2022 15.28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present the prerequisite packages for 64-bit and 32-bit builds
> are slightly different. Let's use the same packages for both.

Not sure whether that's a good idea ... I did that on purpose to save some 
few time during compilation (since the Windows jobs are running very long 
already) ... did you check whether it makes a difference in the run time now?

  Thomas

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Mark Cave-Ayland]

On 08/09/2022 14:28, Bin Meng wrote:

> From: Bin Meng <bin.meng@windriver.com>
> 
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
> 
> Actually build/config-host.mak contains a GLIB_BINDIR variable
> which is the directory where glib and other DLLs reside. This
> works for both Windows native build and cross-build on Linux.
> We can use it as the search directory for DLLs and automate
> the whole DLL packaging process.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   meson.build     |  1 +
>   scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
>   2 files changed, 43 insertions(+), 4 deletions(-)
> 
> diff --git a/meson.build b/meson.build
> index c2adb7caf4..4c03850f9f 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -3657,6 +3657,7 @@ if host_machine.system() == 'windows'
>       '@OUTPUT@',
>       get_option('prefix'),
>       meson.current_source_dir(),
> +    config_host['GLIB_BINDIR'],
>       host_machine.cpu(),
>       '--',
>       '-DDISPLAYVERSION=' + meson.project_version(),
> diff --git a/scripts/nsis.py b/scripts/nsis.py
> index baa6ef9594..03ed7608a2 100644
> --- a/scripts/nsis.py
> +++ b/scripts/nsis.py
> @@ -18,12 +18,36 @@ def signcode(path):
>           return
>       subprocess.run([cmd, path])
>   
> +def find_deps(exe_or_dll, search_path, analyzed_deps):
> +    deps = [exe_or_dll]
> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
> +    output = output.split("\n")
> +    for line in output:
> +        if not line.startswith("\tDLL Name: "):
> +            continue
> +
> +        dep = line.split("DLL Name: ")[1].strip()
> +        if dep in analyzed_deps:
> +            continue
> +
> +        dll = os.path.join(search_path, dep)
> +        if not os.path.exists(dll):
> +            # assume it's a Windows provided dll, skip it
> +            continue
> +
> +        analyzed_deps.add(dep)
> +        # locate the dll dependencies recursively
> +        rdeps = find_deps(dll, search_path, analyzed_deps)
> +        deps.extend(rdeps)
> +
> +    return deps
>   
>   def main():
>       parser = argparse.ArgumentParser(description="QEMU NSIS build helper.")
>       parser.add_argument("outfile")
>       parser.add_argument("prefix")
>       parser.add_argument("srcdir")
> +    parser.add_argument("dlldir")
>       parser.add_argument("cpu")
>       parser.add_argument("nsisargs", nargs="*")
>       args = parser.parse_args()
> @@ -63,9 +87,26 @@ def main():
>                   !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
>                   """.format(arch, desc))
>   
> +        search_path = args.dlldir
> +        print("Searching '%s' for the dependent dlls ..." % search_path)
> +        dlldir = os.path.join(destdir + prefix, "dll")
> +        os.mkdir(dlldir)
> +
>           for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
>               signcode(exe)
>   
> +            # find all dll dependencies
> +            deps = set(find_deps(exe, search_path, set()))
> +            deps.remove(exe)
> +
> +            # copy all dlls to the DLLDIR
> +            for dep in deps:
> +                dllfile = os.path.join(dlldir, os.path.basename(dep))
> +                if (os.path.exists(dllfile)):
> +                    continue
> +                print("Copying '%s' to '%s'" % (dep, dllfile))
> +                shutil.copy(dep, dllfile)
> +
>           makensis = [
>               "makensis",
>               "-V2",
> @@ -73,12 +114,9 @@ def main():
>               "-DSRCDIR=" + args.srcdir,
>               "-DBINDIR=" + destdir + prefix,
>           ]
> -        dlldir = "w32"
>           if args.cpu == "x86_64":
> -            dlldir = "w64"
>               makensis += ["-DW64"]
> -        if os.path.exists(os.path.join(args.srcdir, "dll")):
> -            makensis += ["-DDLLDIR={0}/dll/{1}".format(args.srcdir, dlldir)]
> +        makensis += ["-DDLLDIR=" + dlldir]
>   
>           makensis += ["-DOUTFILE=" + args.outfile] + args.nsisargs
>           subprocess.run(makensis)

FWIW I wrote a similar script a while back to help package a custom Windows build for 
a client, however I used ldd instead of objdump since it provided the full paths for 
DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the 
QEMU build tree.

Once the complete list of DLLs was obtained, it was simple matter of filtering out 
those DLLs that started with the %WINDIR% prefix before copying them to the final 
distribution directory.


ATB,

Mark.

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Bin Meng]

On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
>
> On 08/09/2022 15.28, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present the prerequisite packages for 64-bit and 32-bit builds
> > are slightly different. Let's use the same packages for both.
>
> Not sure whether that's a good idea ... I did that on purpose to save some
> few time during compilation (since the Windows jobs are running very long
> already) ... did you check whether it makes a difference in the run time now?
>

Not much difference on the build time. Actually I found after we
switched to single thread build the time did not increase too.

One side note regarding the gitlab shared runner:

It seems the shared runner Windows VM is quite slow. Is it possible to
get a faster VM externally?

Regards,
Bin

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Bin Meng]

On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
<mark.cave-ayland@ilande.co.uk> wrote:
>
> On 08/09/2022 14:28, Bin Meng wrote:
>
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > At present packaging the required DLLs of QEMU executables is a
> > manual process, and error prone.
> >
> > Actually build/config-host.mak contains a GLIB_BINDIR variable
> > which is the directory where glib and other DLLs reside. This
> > works for both Windows native build and cross-build on Linux.
> > We can use it as the search directory for DLLs and automate
> > the whole DLL packaging process.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >   meson.build     |  1 +
> >   scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
> >   2 files changed, 43 insertions(+), 4 deletions(-)
> >
> > diff --git a/meson.build b/meson.build
> > index c2adb7caf4..4c03850f9f 100644
> > --- a/meson.build
> > +++ b/meson.build
> > @@ -3657,6 +3657,7 @@ if host_machine.system() == 'windows'
> >       '@OUTPUT@',
> >       get_option('prefix'),
> >       meson.current_source_dir(),
> > +    config_host['GLIB_BINDIR'],
> >       host_machine.cpu(),
> >       '--',
> >       '-DDISPLAYVERSION=' + meson.project_version(),
> > diff --git a/scripts/nsis.py b/scripts/nsis.py
> > index baa6ef9594..03ed7608a2 100644
> > --- a/scripts/nsis.py
> > +++ b/scripts/nsis.py
> > @@ -18,12 +18,36 @@ def signcode(path):
> >           return
> >       subprocess.run([cmd, path])
> >
> > +def find_deps(exe_or_dll, search_path, analyzed_deps):
> > +    deps = [exe_or_dll]
> > +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
> > +    output = output.split("\n")
> > +    for line in output:
> > +        if not line.startswith("\tDLL Name: "):
> > +            continue
> > +
> > +        dep = line.split("DLL Name: ")[1].strip()
> > +        if dep in analyzed_deps:
> > +            continue
> > +
> > +        dll = os.path.join(search_path, dep)
> > +        if not os.path.exists(dll):
> > +            # assume it's a Windows provided dll, skip it
> > +            continue
> > +
> > +        analyzed_deps.add(dep)
> > +        # locate the dll dependencies recursively
> > +        rdeps = find_deps(dll, search_path, analyzed_deps)
> > +        deps.extend(rdeps)
> > +
> > +    return deps
> >
> >   def main():
> >       parser = argparse.ArgumentParser(description="QEMU NSIS build helper.")
> >       parser.add_argument("outfile")
> >       parser.add_argument("prefix")
> >       parser.add_argument("srcdir")
> > +    parser.add_argument("dlldir")
> >       parser.add_argument("cpu")
> >       parser.add_argument("nsisargs", nargs="*")
> >       args = parser.parse_args()
> > @@ -63,9 +87,26 @@ def main():
> >                   !insertmacro MUI_DESCRIPTION_TEXT ${{Section_{0}}} "{1}"
> >                   """.format(arch, desc))
> >
> > +        search_path = args.dlldir
> > +        print("Searching '%s' for the dependent dlls ..." % search_path)
> > +        dlldir = os.path.join(destdir + prefix, "dll")
> > +        os.mkdir(dlldir)
> > +
> >           for exe in glob.glob(os.path.join(destdir + prefix, "*.exe")):
> >               signcode(exe)
> >
> > +            # find all dll dependencies
> > +            deps = set(find_deps(exe, search_path, set()))
> > +            deps.remove(exe)
> > +
> > +            # copy all dlls to the DLLDIR
> > +            for dep in deps:
> > +                dllfile = os.path.join(dlldir, os.path.basename(dep))
> > +                if (os.path.exists(dllfile)):
> > +                    continue
> > +                print("Copying '%s' to '%s'" % (dep, dllfile))
> > +                shutil.copy(dep, dllfile)
> > +
> >           makensis = [
> >               "makensis",
> >               "-V2",
> > @@ -73,12 +114,9 @@ def main():
> >               "-DSRCDIR=" + args.srcdir,
> >               "-DBINDIR=" + destdir + prefix,
> >           ]
> > -        dlldir = "w32"
> >           if args.cpu == "x86_64":
> > -            dlldir = "w64"
> >               makensis += ["-DW64"]
> > -        if os.path.exists(os.path.join(args.srcdir, "dll")):
> > -            makensis += ["-DDLLDIR={0}/dll/{1}".format(args.srcdir, dlldir)]
> > +        makensis += ["-DDLLDIR=" + dlldir]
> >
> >           makensis += ["-DOUTFILE=" + args.outfile] + args.nsisargs
> >           subprocess.run(makensis)
>
> FWIW I wrote a similar script a while back to help package a custom Windows build for
> a client, however I used ldd instead of objdump since it provided the full paths for
> DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the
> QEMU build tree.
>

Yep, ldd also works, but only on Windows native build. objdump can
work on both Windows native and Linux cross builds.

> Once the complete list of DLLs was obtained, it was simple matter of filtering out
> those DLLs that started with the %WINDIR% prefix before copying them to the final
> distribution directory.
>

Regards,
Bin

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[罗勇刚(Yonggang Luo)]

[-- Attachment #1: Type: text/plain, Size: 1253 bytes --]

How about using github actions, I tried before and it's running fast, there
is no resource restriction.
Just don't know how to trigger it through gitlab, if that's possible, then
it's would be good

On Sat, Sep 10, 2022 at 8:33 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 08/09/2022 15.28, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > At present the prerequisite packages for 64-bit and 32-bit builds
> > > are slightly different. Let's use the same packages for both.
> >
> > Not sure whether that's a good idea ... I did that on purpose to save
some
> > few time during compilation (since the Windows jobs are running very
long
> > already) ... did you check whether it makes a difference in the run
time now?
> >
>
> Not much difference on the build time. Actually I found after we
> switched to single thread build the time did not increase too.
>
> One side note regarding the gitlab shared runner:
>
> It seems the shared runner Windows VM is quite slow. Is it possible to
> get a faster VM externally?
>
> Regards,
> Bin
>


--
         此致
礼
罗勇刚
Yours
    sincerely,
Yonggang Luo

[-- Attachment #2: Type: text/html, Size: 1638 bytes --]

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Bin Meng]

On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
>
> Improve scripts/nsis.py by adding a logic to automatically package
> required DLLs of QEMU executables.
>
> 'make installer' is tested in the cross-build on Linux in CI, but
> not in the Windows native build. Update CI to test the installer
> generation on Windows too.
>
> During testing a 32-bit build issue was exposed in block/nfs.c and
> the fix is included in this series.
>
>
> Bin Meng (7):
>   scripts/nsis.py: Drop the unnecessary path separator
>   scripts/nsis.py: Fix destination directory name when invoked on
>     Windows
>   scripts/nsis.py: Automatically package required DLLs of QEMU
>     executables
>   .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
>   block/nfs: Fix 32-bit Windows build
>   .gitlab-ci.d/windows.yml: Unify the prerequisite packages
>   .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
>
>  meson.build              |  1 +
>  block/nfs.c              |  8 ++++++
>  .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
>  scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
>  4 files changed, 89 insertions(+), 20 deletions(-)
>

Ping for this series?

Re: [PATCH 1/7] scripts/nsis.py: Drop the unnecessary path separator

[Philippe Mathieu-Daudé via]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> There is no need to append a path separator to the destination
> directory that is passed to "make install".
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   scripts/nsis.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 2/7] scripts/nsis.py: Fix destination directory name when invoked on Windows

[Philippe Mathieu-Daudé via]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> "make installer" on Windows fails with the following message:
> 
>    Traceback (most recent call last):
>      File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 89, in <module>
>        main()
>      File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 34, in main
>        with open(
>    OSError: [Errno 22] Invalid argument:
>    'R:/Temp/tmpw83xhjquG:/msys64/qemu/system-emulations.nsh'
>    ninja: build stopped: subcommand failed.
> 
> Use os.path.splitdrive() to form a canonical path without the drive
> letter on Windows. This works with cross-build on Linux too.
> 
> Fixes: 8adfeba953e0 ("meson: add NSIS building")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   scripts/nsis.py | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 4/7] .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build

[Philippe Mathieu-Daudé via]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> The sed processing of build/config-host.mak seems to be no longer
> needed, and there is no such in the 32-bit build too. Drop it.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   .gitlab-ci.d/windows.yml | 1 -
>   1 file changed, 1 deletion(-)

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 7/7] .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

[Philippe Mathieu-Daudé via]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Now that we have supported packaging DLLs automatically, let's add
> the 'make installer' in the CI and publish the generated installer
> file as an artifact.
> 
> Increase the job timeout to 90 minutes to accommodate to it.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   .gitlab-ci.d/windows.yml | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
> index fffb202658..3a94d40e73 100644
> --- a/.gitlab-ci.d/windows.yml
> +++ b/.gitlab-ci.d/windows.yml
> @@ -10,7 +10,7 @@
>         - ${CI_PROJECT_DIR}/msys64/var/cache
>     needs: []
>     stage: build
> -  timeout: 70m
> +  timeout: 90m
>     before_script:
>     - If ( !(Test-Path -Path msys64\var\cache ) ) {
>         mkdir msys64\var\cache
> @@ -28,6 +28,11 @@
>     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
>     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
>     - taskkill /F /FI "MODULES eq msys-2.0.dll"
> +  artifacts:
> +    name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
> +    expire_in: 7 days
> +    paths:
> +      - build/qemu-setup*.exe

Do you really want to test this binary? I think the CI is only to test
the installer. This is a stripped down version anyway (./configure
options). If someone want to package/test, this should not be done here
but locally.

However I agree testing the installer doesn't bitrot is helpful, so
*without* the "artifacts" section:

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Philippe Mathieu-Daudé via]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
> 
> libnfs.h declares nfs_fstat() as the following for win32:
> 
>    int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
>                  struct __stat64 *st);
> 
> The 'st' parameter should be of type 'struct __stat64'. The
> codes happen to build successfully for 64-bit Windows, but it
> does not build for 32-bit Windows.
> 
> Fixes: 6542aa9c75bc ("block: add native support for NFS")
> Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for read-only files")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   block/nfs.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index 444c40b458..d5d67937dd 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
>                                  int flags, int open_flags, Error **errp)
>   {
>       int64_t ret = -EINVAL;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       char *file = NULL, *strp = NULL;
>   
>       qemu_mutex_init(&client->mutex);
> @@ -781,7 +785,11 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
>                                 BlockReopenQueue *queue, Error **errp)
>   {
>       NFSClient *client = state->bs->opaque;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       int ret = 0;
>   
>       if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {

What about the field in struct NFSRPC?

RE: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Meng, Bin]

-----Original Message-----
From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of Philippe Mathieu-Daudé
Sent: Sunday, September 18, 2022 5:32 AM
To: Bin Meng <bmeng.cn@gmail.com>; qemu-devel@nongnu.org
Cc: Meng, Bin <Bin.Meng@windriver.com>; Hanna Reitz <hreitz@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Peter Lieven <pl@kamp.de>; qemu-block@nongnu.org
Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Please note: This e-mail is from an EXTERNAL e-mail address]

On 8/9/22 15:28, Bin Meng wrote:
> From: Bin Meng <bin.meng@windriver.com>
>
> libnfs.h declares nfs_fstat() as the following for win32:
>
>    int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
>                  struct __stat64 *st);
>
> The 'st' parameter should be of type 'struct __stat64'. The codes 
> happen to build successfully for 64-bit Windows, but it does not build 
> for 32-bit Windows.
>
> Fixes: 6542aa9c75bc ("block: add native support for NFS")
> Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for 
> read-only files")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
>
>   block/nfs.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
>
> diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd 
> 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
>                                  int flags, int open_flags, Error **errp)
>   {
>       int64_t ret = -EINVAL;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       char *file = NULL, *strp = NULL;
>
>       qemu_mutex_init(&client->mutex); @@ -781,7 +785,11 @@ static int 
> nfs_reopen_prepare(BDRVReopenState *state,
>                                 BlockReopenQueue *queue, Error **errp)
>   {
>       NFSClient *client = state->bs->opaque;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       int ret = 0;
>
>       if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) 
> {

What about the field in struct NFSRPC?

NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and nfs_get_allocated_file_size() that are not built on win32, so there is no problem.

Regards,
Bin

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Bin Meng]

Hi,

On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
>
> Improve scripts/nsis.py by adding a logic to automatically package
> required DLLs of QEMU executables.
>
> 'make installer' is tested in the cross-build on Linux in CI, but
> not in the Windows native build. Update CI to test the installer
> generation on Windows too.
>
> During testing a 32-bit build issue was exposed in block/nfs.c and
> the fix is included in this series.
>
>
> Bin Meng (7):
>   scripts/nsis.py: Drop the unnecessary path separator
>   scripts/nsis.py: Fix destination directory name when invoked on
>     Windows
>   scripts/nsis.py: Automatically package required DLLs of QEMU
>     executables
>   .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
>   block/nfs: Fix 32-bit Windows build
>   .gitlab-ci.d/windows.yml: Unify the prerequisite packages
>   .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
>
>  meson.build              |  1 +
>  block/nfs.c              |  8 ++++++
>  .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
>  scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
>  4 files changed, 89 insertions(+), 20 deletions(-)
>

I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
sed processing in the 64-bit build")

What about other patches?

Regards,
Bin

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Thomas Huth]

On 21/09/2022 14.18, Bin Meng wrote:
> Hi,
> 
> On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> At present packaging the required DLLs of QEMU executables is a
>> manual process, and error prone.
>>
>> Improve scripts/nsis.py by adding a logic to automatically package
>> required DLLs of QEMU executables.
>>
>> 'make installer' is tested in the cross-build on Linux in CI, but
>> not in the Windows native build. Update CI to test the installer
>> generation on Windows too.
>>
>> During testing a 32-bit build issue was exposed in block/nfs.c and
>> the fix is included in this series.
>>
>>
>> Bin Meng (7):
>>    scripts/nsis.py: Drop the unnecessary path separator
>>    scripts/nsis.py: Fix destination directory name when invoked on
>>      Windows
>>    scripts/nsis.py: Automatically package required DLLs of QEMU
>>      executables
>>    .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
>>    block/nfs: Fix 32-bit Windows build
>>    .gitlab-ci.d/windows.yml: Unify the prerequisite packages
>>    .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
>>
>>   meson.build              |  1 +
>>   block/nfs.c              |  8 ++++++
>>   .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
>>   scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
>>   4 files changed, 89 insertions(+), 20 deletions(-)
>>
> 
> I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> sed processing in the 64-bit build")
> 
> What about other patches?

I hope that Stefan Weil (our W32 maintainer) could have a look at these first...

  Thomas

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Bin Meng]

Hi Stefan,

On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 21/09/2022 14.18, Bin Meng wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> At present packaging the required DLLs of QEMU executables is a
> >> manual process, and error prone.
> >>
> >> Improve scripts/nsis.py by adding a logic to automatically package
> >> required DLLs of QEMU executables.
> >>
> >> 'make installer' is tested in the cross-build on Linux in CI, but
> >> not in the Windows native build. Update CI to test the installer
> >> generation on Windows too.
> >>
> >> During testing a 32-bit build issue was exposed in block/nfs.c and
> >> the fix is included in this series.
> >>
> >>
> >> Bin Meng (7):
> >>    scripts/nsis.py: Drop the unnecessary path separator
> >>    scripts/nsis.py: Fix destination directory name when invoked on
> >>      Windows
> >>    scripts/nsis.py: Automatically package required DLLs of QEMU
> >>      executables
> >>    .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> >>    block/nfs: Fix 32-bit Windows build
> >>    .gitlab-ci.d/windows.yml: Unify the prerequisite packages
> >>    .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
> >>
> >>   meson.build              |  1 +
> >>   block/nfs.c              |  8 ++++++
> >>   .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
> >>   scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
> >>   4 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >
> > I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> > sed processing in the 64-bit build")
> >
> > What about other patches?
>
> I hope that Stefan Weil (our W32 maintainer) could have a look at these first...

Would you please comment this series? Thanks!

Regards,
Bin

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Bin Meng]

Hi,

On Wed, Sep 21, 2022 at 8:10 PM Meng, Bin <Bin.Meng@windriver.com> wrote:
>
> -----Original Message-----
> From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of Philippe Mathieu-Daudé
> Sent: Sunday, September 18, 2022 5:32 AM
> To: Bin Meng <bmeng.cn@gmail.com>; qemu-devel@nongnu.org
> Cc: Meng, Bin <Bin.Meng@windriver.com>; Hanna Reitz <hreitz@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Peter Lieven <pl@kamp.de>; qemu-block@nongnu.org
> Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
>
> [Please note: This e-mail is from an EXTERNAL e-mail address]
>
> On 8/9/22 15:28, Bin Meng wrote:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > libnfs.h declares nfs_fstat() as the following for win32:
> >
> >    int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
> >                  struct __stat64 *st);
> >
> > The 'st' parameter should be of type 'struct __stat64'. The codes
> > happen to build successfully for 64-bit Windows, but it does not build
> > for 32-bit Windows.
> >
> > Fixes: 6542aa9c75bc ("block: add native support for NFS")
> > Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for
> > read-only files")
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >   block/nfs.c | 8 ++++++++
> >   1 file changed, 8 insertions(+)
> >
> > diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd
> > 100644
> > --- a/block/nfs.c
> > +++ b/block/nfs.c
> > @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
> >                                  int flags, int open_flags, Error **errp)
> >   {
> >       int64_t ret = -EINVAL;
> > +#ifdef _WIN32
> > +    struct __stat64 st;
> > +#else
> >       struct stat st;
> > +#endif
> >       char *file = NULL, *strp = NULL;
> >
> >       qemu_mutex_init(&client->mutex); @@ -781,7 +785,11 @@ static int
> > nfs_reopen_prepare(BDRVReopenState *state,
> >                                 BlockReopenQueue *queue, Error **errp)
> >   {
> >       NFSClient *client = state->bs->opaque;
> > +#ifdef _WIN32
> > +    struct __stat64 st;
> > +#else
> >       struct stat st;
> > +#endif
> >       int ret = 0;
> >
> >       if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs))
> > {
>
> What about the field in struct NFSRPC?
>
> NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and nfs_get_allocated_file_size() that are not built on win32, so there is no problem.
>

Any further comments?

Regards,
Bin

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Bin Meng]

Hi Thomas,

On Sat, Sep 10, 2022 at 8:32 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
> >
> > On 08/09/2022 15.28, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > At present the prerequisite packages for 64-bit and 32-bit builds
> > > are slightly different. Let's use the same packages for both.
> >
> > Not sure whether that's a good idea ... I did that on purpose to save some
> > few time during compilation (since the Windows jobs are running very long
> > already) ... did you check whether it makes a difference in the run time now?
> >
>
> Not much difference on the build time. Actually I found after we
> switched to single thread build the time did not increase too.
>
> One side note regarding the gitlab shared runner:
>
> It seems the shared runner Windows VM is quite slow. Is it possible to
> get a faster VM externally?

Any further comment for this patch?

Regards,
Bin

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Bin Meng]

Hi Kevin,

On Sat, Sep 24, 2022 at 9:19 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi,
>
> On Wed, Sep 21, 2022 at 8:10 PM Meng, Bin <Bin.Meng@windriver.com> wrote:
> >
> > -----Original Message-----
> > From: Philippe Mathieu-Daudé <philippe.mathieu.daude@gmail.com> On Behalf Of Philippe Mathieu-Daudé
> > Sent: Sunday, September 18, 2022 5:32 AM
> > To: Bin Meng <bmeng.cn@gmail.com>; qemu-devel@nongnu.org
> > Cc: Meng, Bin <Bin.Meng@windriver.com>; Hanna Reitz <hreitz@redhat.com>; Kevin Wolf <kwolf@redhat.com>; Peter Lieven <pl@kamp.de>; qemu-block@nongnu.org
> > Subject: Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build
> >
> > [Please note: This e-mail is from an EXTERNAL e-mail address]
> >
> > On 8/9/22 15:28, Bin Meng wrote:
> > > From: Bin Meng <bin.meng@windriver.com>
> > >
> > > libnfs.h declares nfs_fstat() as the following for win32:
> > >
> > >    int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
> > >                  struct __stat64 *st);
> > >
> > > The 'st' parameter should be of type 'struct __stat64'. The codes
> > > happen to build successfully for 64-bit Windows, but it does not build
> > > for 32-bit Windows.
> > >
> > > Fixes: 6542aa9c75bc ("block: add native support for NFS")
> > > Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for
> > > read-only files")
> > > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > > ---
> > >
> > >   block/nfs.c | 8 ++++++++
> > >   1 file changed, 8 insertions(+)
> > >
> > > diff --git a/block/nfs.c b/block/nfs.c index 444c40b458..d5d67937dd
> > > 100644
> > > --- a/block/nfs.c
> > > +++ b/block/nfs.c
> > > @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
> > >                                  int flags, int open_flags, Error **errp)
> > >   {
> > >       int64_t ret = -EINVAL;
> > > +#ifdef _WIN32
> > > +    struct __stat64 st;
> > > +#else
> > >       struct stat st;
> > > +#endif
> > >       char *file = NULL, *strp = NULL;
> > >
> > >       qemu_mutex_init(&client->mutex); @@ -781,7 +785,11 @@ static int
> > > nfs_reopen_prepare(BDRVReopenState *state,
> > >                                 BlockReopenQueue *queue, Error **errp)
> > >   {
> > >       NFSClient *client = state->bs->opaque;
> > > +#ifdef _WIN32
> > > +    struct __stat64 st;
> > > +#else
> > >       struct stat st;
> > > +#endif
> > >       int ret = 0;
> > >
> > >       if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs))
> > > {
> >
> > What about the field in struct NFSRPC?
> >
> > NFSRPC::struct stat is used in nfs_get_allocated_file_size_cb() and nfs_get_allocated_file_size() that are not built on win32, so there is no problem.
> >
>
> Any further comments?
>

Will you queue this patch via the block tree?

Regards,
Bin

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Kevin Wolf]

Am 27.10.2022 um 04:45 hat Bin Meng geschrieben:
> Hi Kevin,
> [...]
> Will you queue this patch via the block tree?

Just to be sure, you mean only patch 5? Yes, I can do that.

Kevin

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Bin Meng]

On Thu, Oct 27, 2022 at 3:55 PM Kevin Wolf <kwolf@redhat.com> wrote:
>
> Am 27.10.2022 um 04:45 hat Bin Meng geschrieben:
> > Hi Kevin,
> > [...]
> > Will you queue this patch via the block tree?
>
> Just to be sure, you mean only patch 5? Yes, I can do that.
>

Yes, only this one. Thank you.

Regards,
Bin

Re: [PATCH 1/7] scripts/nsis.py: Drop the unnecessary path separator

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 915 bytes --]

Am 08.09.22 um 15:28 schrieb Bin Meng:
> From: Bin Meng <bin.meng@windriver.com>
> 
> There is no need to append a path separator to the destination
> directory that is passed to "make install".
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   scripts/nsis.py | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/nsis.py b/scripts/nsis.py
> index 462d6cac3b..bbb41d9386 100644
> --- a/scripts/nsis.py
> +++ b/scripts/nsis.py
> @@ -30,7 +30,7 @@ def main():
>   
>       destdir = tempfile.mkdtemp()
>       try:
> -        subprocess.run(["make", "install", "DESTDIR=" + destdir + os.path.sep])
> +        subprocess.run(["make", "install", "DESTDIR=" + destdir])
>           with open(
>               os.path.join(destdir + args.prefix, "system-emulations.nsh"), "w"
>           ) as nsh, open(

Reviewed-by: Stefan Weil <sw@weilnetz.de>

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 2/7] scripts/nsis.py: Fix destination directory name when invoked on Windows

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 945 bytes --]

Am 08.09.22 um 15:28 schrieb Bin Meng:
> From: Bin Meng <bin.meng@windriver.com>
> 
> "make installer" on Windows fails with the following message:
> 
>    Traceback (most recent call last):
>      File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 89, in <module>
>        main()
>      File "G:\msys64\home\foo\git\qemu\scripts\nsis.py", line 34, in main
>        with open(
>    OSError: [Errno 22] Invalid argument:
>    'R:/Temp/tmpw83xhjquG:/msys64/qemu/system-emulations.nsh'
>    ninja: build stopped: subcommand failed.
> 
> Use os.path.splitdrive() to form a canonical path without the drive
> letter on Windows. This works with cross-build on Linux too.
> 
> Fixes: 8adfeba953e0 ("meson: add NSIS building")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   scripts/nsis.py | 12 +++++++-----
>   1 file changed, 7 insertions(+), 5 deletions(-)
> 

Tested-by: Stefan Weil <sw@weilnetz.de>

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Stefan Weil via]

Am 08.09.22 um 15:28 schrieb Bin Meng:
> From: Bin Meng <bin.meng@windriver.com>
> 
> At present packaging the required DLLs of QEMU executables is a
> manual process, and error prone.
> 
> Actually build/config-host.mak contains a GLIB_BINDIR variable
> which is the directory where glib and other DLLs reside. This
> works for both Windows native build and cross-build on Linux.
> We can use it as the search directory for DLLs and automate
> the whole DLL packaging process.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---


Thank you, that is really helpful to avoid broken installers because of 
missing DLL files.

Tested-by: Stefan Weil <sw@weilnetz.de>

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Bin Meng]

Hi Thomas,

On Sat, Sep 24, 2022 at 5:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>
> Hi Thomas,
>
> On Sat, Sep 10, 2022 at 8:32 AM Bin Meng <bmeng.cn@gmail.com> wrote:
> >
> > On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
> > >
> > > On 08/09/2022 15.28, Bin Meng wrote:
> > > > From: Bin Meng <bin.meng@windriver.com>
> > > >
> > > > At present the prerequisite packages for 64-bit and 32-bit builds
> > > > are slightly different. Let's use the same packages for both.
> > >
> > > Not sure whether that's a good idea ... I did that on purpose to save some
> > > few time during compilation (since the Windows jobs are running very long
> > > already) ... did you check whether it makes a difference in the run time now?
> > >
> >
> > Not much difference on the build time. Actually I found after we
> > switched to single thread build the time did not increase too.
> >
> > One side note regarding the gitlab shared runner:
> >
> > It seems the shared runner Windows VM is quite slow. Is it possible to
> > get a faster VM externally?
>
> Any further comment for this patch?
>

Ping?

Regards,
Bin

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Bin Meng]

Hi Thomas,

On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth <thuth@redhat.com> wrote:
>
> On 21/09/2022 14.18, Bin Meng wrote:
> > Hi,
> >
> > On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
> >>
> >> At present packaging the required DLLs of QEMU executables is a
> >> manual process, and error prone.
> >>
> >> Improve scripts/nsis.py by adding a logic to automatically package
> >> required DLLs of QEMU executables.
> >>
> >> 'make installer' is tested in the cross-build on Linux in CI, but
> >> not in the Windows native build. Update CI to test the installer
> >> generation on Windows too.
> >>
> >> During testing a 32-bit build issue was exposed in block/nfs.c and
> >> the fix is included in this series.
> >>
> >>
> >> Bin Meng (7):
> >>    scripts/nsis.py: Drop the unnecessary path separator
> >>    scripts/nsis.py: Fix destination directory name when invoked on
> >>      Windows
> >>    scripts/nsis.py: Automatically package required DLLs of QEMU
> >>      executables
> >>    .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
> >>    block/nfs: Fix 32-bit Windows build
> >>    .gitlab-ci.d/windows.yml: Unify the prerequisite packages
> >>    .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
> >>
> >>   meson.build              |  1 +
> >>   block/nfs.c              |  8 ++++++
> >>   .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
> >>   scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
> >>   4 files changed, 89 insertions(+), 20 deletions(-)
> >>
> >
> > I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
> > sed processing in the 64-bit build")
> >
> > What about other patches?
>
> I hope that Stefan Weil (our W32 maintainer) could have a look at these first...
>

Stefan has reviewed / tested patch 1-3. Not sure who is going to queue
these 3 patches?

Regards,
Bin

Re: [PATCH 5/7] block/nfs: Fix 32-bit Windows build

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 1695 bytes --]

Am 08.09.22 um 15:28 schrieb Bin Meng:
> From: Bin Meng <bin.meng@windriver.com>
> 
> libnfs.h declares nfs_fstat() as the following for win32:
> 
>    int nfs_fstat(struct nfs_context *nfs, struct nfsfh *nfsfh,
>                  struct __stat64 *st);
> 
> The 'st' parameter should be of type 'struct __stat64'. The
> codes happen to build successfully for 64-bit Windows, but it
> does not build for 32-bit Windows.
> 
> Fixes: 6542aa9c75bc ("block: add native support for NFS")
> Fixes: 18a8056e0bc7 ("block/nfs: cache allocated filesize for read-only files")
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   block/nfs.c | 8 ++++++++
>   1 file changed, 8 insertions(+)
> 
> diff --git a/block/nfs.c b/block/nfs.c
> index 444c40b458..d5d67937dd 100644
> --- a/block/nfs.c
> +++ b/block/nfs.c
> @@ -418,7 +418,11 @@ static int64_t nfs_client_open(NFSClient *client, BlockdevOptionsNfs *opts,
>                                  int flags, int open_flags, Error **errp)
>   {
>       int64_t ret = -EINVAL;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       char *file = NULL, *strp = NULL;
>   
>       qemu_mutex_init(&client->mutex);
> @@ -781,7 +785,11 @@ static int nfs_reopen_prepare(BDRVReopenState *state,
>                                 BlockReopenQueue *queue, Error **errp)
>   {
>       NFSClient *client = state->bs->opaque;
> +#ifdef _WIN32
> +    struct __stat64 st;
> +#else
>       struct stat st;
> +#endif
>       int ret = 0;
>   
>       if (state->flags & BDRV_O_RDWR && bdrv_is_read_only(state->bs)) {


Thanks!

Reviewed-by: Stefan Weil <sw@weilnetz.de>


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 2398 bytes --]

Am 29.10.22 um 15:06 schrieb Bin Meng:
> Hi Thomas,
> 
> On Sat, Sep 24, 2022 at 5:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Thomas,
>>
>> On Sat, Sep 10, 2022 at 8:32 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 08/09/2022 15.28, Bin Meng wrote:
>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> At present the prerequisite packages for 64-bit and 32-bit builds
>>>>> are slightly different. Let's use the same packages for both.
>>>>
>>>> Not sure whether that's a good idea ... I did that on purpose to save some
>>>> few time during compilation (since the Windows jobs are running very long
>>>> already) ... did you check whether it makes a difference in the run time now?
>>>>
>>>
>>> Not much difference on the build time. Actually I found after we
>>> switched to single thread build the time did not increase too.
>>>
>>> One side note regarding the gitlab shared runner:
>>>
>>> It seems the shared runner Windows VM is quite slow. Is it possible to
>>> get a faster VM externally?
>>
>> Any further comment for this patch?
>>
> 
> Ping?
> 
> Regards,
> Bin


Adding more packages typically is a good idea because it allows 
compilation of more code features, so the coverage during the build is 
increased.

But here we have 32 and 64 bit builds for Windows which are less 
different than on Linux because sizeof(long) is the same for both. 
Nevertheless patch 5 of the series shows an example where 32 bit builds 
produced a warning, but 64 bit builds did not. So covering as much code 
as possible (which requires installation of all required packages) can 
be useful for build tests. And of course it is also necessary if the 
generated binaries should support as many features as possible.

On the other hand builds with a reduced number of packages can also be 
reasonable, not only because they need less resources (build time, 
energy), but also because they can uncover broken dependencies. And for 
some applications smaller binaries with less features can be sufficient.

So there is no clear answer for me whether the packages for 32 and 64 
bit should be synchronized or not.

I think technically the patch is fine. We can postpone the decision 
because it is not urgent for the upcoming release.

Stefan


[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 7/7] .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

[Stefan Weil via]

[-- Attachment #1.1.1: Type: text/plain, Size: 4117 bytes --]

Am 08.09.22 um 15:28 schrieb Bin Meng:
> From: Bin Meng <bin.meng@windriver.com>
> 
> Now that we have supported packaging DLLs automatically, let's add
> the 'make installer' in the CI and publish the generated installer
> file as an artifact.
> 
> Increase the job timeout to 90 minutes to accommodate to it.
> 
> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> ---
> 
>   .gitlab-ci.d/windows.yml | 27 +++++++++++++++++++--------
>   1 file changed, 19 insertions(+), 8 deletions(-)
> 
> diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
> index fffb202658..3a94d40e73 100644
> --- a/.gitlab-ci.d/windows.yml
> +++ b/.gitlab-ci.d/windows.yml
> @@ -10,7 +10,7 @@
>         - ${CI_PROJECT_DIR}/msys64/var/cache
>     needs: []
>     stage: build
> -  timeout: 70m
> +  timeout: 90m
>     before_script:
>     - If ( !(Test-Path -Path msys64\var\cache ) ) {
>         mkdir msys64\var\cache
> @@ -28,6 +28,11 @@
>     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
>     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
>     - taskkill /F /FI "MODULES eq msys-2.0.dll"
> +  artifacts:
> +    name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
> +    expire_in: 7 days
> +    paths:
> +      - build/qemu-setup*.exe
>   
>   msys2-64bit:
>     extends: .shared_msys2_builder
> @@ -51,6 +56,7 @@ msys2-64bit:
>         mingw-w64-x86_64-lzo2
>         mingw-w64-x86_64-nettle
>         mingw-w64-x86_64-ninja
> +      mingw-w64-x86_64-nsis
>         mingw-w64-x86_64-pixman
>         mingw-w64-x86_64-pkgconf
>         mingw-w64-x86_64-python
> @@ -60,12 +66,15 @@ msys2-64bit:
>         mingw-w64-x86_64-usbredir
>         mingw-w64-x86_64-zstd "
>     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
> -  - $env:MSYSTEM = 'MINGW64'     # Start a 64 bit Mingw environment
> +  - $env:MSYSTEM = 'MINGW64'     # Start a 64-bit MinGW environment

I use Mingw-w64, not MinGW. :-)

https://www.mingw-w64.org/ uses inconsistent case, mostly Mingw-w64, but 
also MinGW-w64. The same confusion exists in the description of the 
Debian packages, but there MinGW-w64 is more common.

So there seems to be no right or wrong.

>     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
> -  - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
> +  - mkdir build
> +  - cd build
> +  - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
>         --enable-capstone --without-default-devices'
> -  - .\msys64\usr\bin\bash -lc 'make'
> -  - .\msys64\usr\bin\bash -lc 'make check'
> +  - ..\msys64\usr\bin\bash -lc 'make'
> +  - ..\msys64\usr\bin\bash -lc 'make check'
> +  - ..\msys64\usr\bin\bash -lc 'make installer'
>   
>   msys2-32bit:
>     extends: .shared_msys2_builder
> @@ -89,6 +98,7 @@ msys2-32bit:
>         mingw-w64-i686-lzo2
>         mingw-w64-i686-nettle
>         mingw-w64-i686-ninja
> +      mingw-w64-i686-nsis
>         mingw-w64-i686-pixman
>         mingw-w64-i686-pkgconf
>         mingw-w64-i686-python
> @@ -98,10 +108,11 @@ msys2-32bit:
>         mingw-w64-i686-usbredir
>         mingw-w64-i686-zstd "
>     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
> -  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinG environment
> +  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinGW environment
>     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
> -  - mkdir output
> -  - cd output
> +  - mkdir build
> +  - cd build
>     - ..\msys64\usr\bin\bash -lc "../configure --target-list=ppc64-softmmu"
>     - ..\msys64\usr\bin\bash -lc 'make'
>     - ..\msys64\usr\bin\bash -lc 'make check'
> +  - ..\msys64\usr\bin\bash -lc 'make installer'

Maybe it is sufficient to build only a 64 bit installer. Is there still 
need for QEMU on 32 bit Windows? For CI, most parts of the NSIS process 
(which requires a lot of resources) are covered by either 32 or 64 bit 
builds, so running both might be unnecessary.

Regards
Stefan

[-- Attachment #1.1.2: OpenPGP public key --]
[-- Type: application/pgp-keys, Size: 6511 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Re: [PATCH 7/7] .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

[Bin Meng]

On Sun, Oct 30, 2022 at 12:39 AM Stefan Weil <sw@weilnetz.de> wrote:
>
> Am 08.09.22 um 15:28 schrieb Bin Meng:
> > From: Bin Meng <bin.meng@windriver.com>
> >
> > Now that we have supported packaging DLLs automatically, let's add
> > the 'make installer' in the CI and publish the generated installer
> > file as an artifact.
> >
> > Increase the job timeout to 90 minutes to accommodate to it.
> >
> > Signed-off-by: Bin Meng <bin.meng@windriver.com>
> > ---
> >
> >   .gitlab-ci.d/windows.yml | 27 +++++++++++++++++++--------
> >   1 file changed, 19 insertions(+), 8 deletions(-)
> >
> > diff --git a/.gitlab-ci.d/windows.yml b/.gitlab-ci.d/windows.yml
> > index fffb202658..3a94d40e73 100644
> > --- a/.gitlab-ci.d/windows.yml
> > +++ b/.gitlab-ci.d/windows.yml
> > @@ -10,7 +10,7 @@
> >         - ${CI_PROJECT_DIR}/msys64/var/cache
> >     needs: []
> >     stage: build
> > -  timeout: 70m
> > +  timeout: 90m
> >     before_script:
> >     - If ( !(Test-Path -Path msys64\var\cache ) ) {
> >         mkdir msys64\var\cache
> > @@ -28,6 +28,11 @@
> >     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Core update
> >     - .\msys64\usr\bin\bash -lc 'pacman --noconfirm -Syuu'  # Normal update
> >     - taskkill /F /FI "MODULES eq msys-2.0.dll"
> > +  artifacts:
> > +    name: "$CI_JOB_NAME-$CI_COMMIT_REF_SLUG"
> > +    expire_in: 7 days
> > +    paths:
> > +      - build/qemu-setup*.exe
> >
> >   msys2-64bit:
> >     extends: .shared_msys2_builder
> > @@ -51,6 +56,7 @@ msys2-64bit:
> >         mingw-w64-x86_64-lzo2
> >         mingw-w64-x86_64-nettle
> >         mingw-w64-x86_64-ninja
> > +      mingw-w64-x86_64-nsis
> >         mingw-w64-x86_64-pixman
> >         mingw-w64-x86_64-pkgconf
> >         mingw-w64-x86_64-python
> > @@ -60,12 +66,15 @@ msys2-64bit:
> >         mingw-w64-x86_64-usbredir
> >         mingw-w64-x86_64-zstd "
> >     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
> > -  - $env:MSYSTEM = 'MINGW64'     # Start a 64 bit Mingw environment
> > +  - $env:MSYSTEM = 'MINGW64'     # Start a 64-bit MinGW environment
>
> I use Mingw-w64, not MinGW. :-)
>
> https://www.mingw-w64.org/ uses inconsistent case, mostly Mingw-w64, but
> also MinGW-w64. The same confusion exists in the description of the
> Debian packages, but there MinGW-w64 is more common.
>
> So there seems to be no right or wrong.

I would suggest we either use mingw-w64, or MinGW-w64 :)

>
> >     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
> > -  - .\msys64\usr\bin\bash -lc './configure --target-list=x86_64-softmmu
> > +  - mkdir build
> > +  - cd build
> > +  - ..\msys64\usr\bin\bash -lc '../configure --target-list=x86_64-softmmu
> >         --enable-capstone --without-default-devices'
> > -  - .\msys64\usr\bin\bash -lc 'make'
> > -  - .\msys64\usr\bin\bash -lc 'make check'
> > +  - ..\msys64\usr\bin\bash -lc 'make'
> > +  - ..\msys64\usr\bin\bash -lc 'make check'
> > +  - ..\msys64\usr\bin\bash -lc 'make installer'
> >
> >   msys2-32bit:
> >     extends: .shared_msys2_builder
> > @@ -89,6 +98,7 @@ msys2-32bit:
> >         mingw-w64-i686-lzo2
> >         mingw-w64-i686-nettle
> >         mingw-w64-i686-ninja
> > +      mingw-w64-i686-nsis
> >         mingw-w64-i686-pixman
> >         mingw-w64-i686-pkgconf
> >         mingw-w64-i686-python
> > @@ -98,10 +108,11 @@ msys2-32bit:
> >         mingw-w64-i686-usbredir
> >         mingw-w64-i686-zstd "
> >     - $env:CHERE_INVOKING = 'yes'  # Preserve the current working directory
> > -  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinG environment
> > +  - $env:MSYSTEM = 'MINGW32'     # Start a 32-bit MinGW environment
> >     - $env:MSYS = 'winsymlinks:native' # Enable native Windows symlink
> > -  - mkdir output
> > -  - cd output
> > +  - mkdir build
> > +  - cd build
> >     - ..\msys64\usr\bin\bash -lc "../configure --target-list=ppc64-softmmu"
> >     - ..\msys64\usr\bin\bash -lc 'make'
> >     - ..\msys64\usr\bin\bash -lc 'make check'
> > +  - ..\msys64\usr\bin\bash -lc 'make installer'
>
> Maybe it is sufficient to build only a 64 bit installer. Is there still
> need for QEMU on 32 bit Windows? For CI, most parts of the NSIS process
> (which requires a lot of resources) are covered by either 32 or 64 bit
> builds, so running both might be unnecessary.

I see no need to support QEMU on 32-bit Windows as it is less common.

Regards,
Bin

Re: [PATCH 6/7] .gitlab-ci.d/windows.yml: Unify the prerequisite packages

[Thomas Huth]

On 29/10/2022 15.06, Bin Meng wrote:
> Hi Thomas,
> 
> On Sat, Sep 24, 2022 at 5:20 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>
>> Hi Thomas,
>>
>> On Sat, Sep 10, 2022 at 8:32 AM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>
>>> On Sat, Sep 10, 2022 at 12:32 AM Thomas Huth <thuth@redhat.com> wrote:
>>>>
>>>> On 08/09/2022 15.28, Bin Meng wrote:
>>>>> From: Bin Meng <bin.meng@windriver.com>
>>>>>
>>>>> At present the prerequisite packages for 64-bit and 32-bit builds
>>>>> are slightly different. Let's use the same packages for both.
>>>>
>>>> Not sure whether that's a good idea ... I did that on purpose to save some
>>>> few time during compilation (since the Windows jobs are running very long
>>>> already) ... did you check whether it makes a difference in the run time now?
>>>>
>>>
>>> Not much difference on the build time. Actually I found after we
>>> switched to single thread build the time did not increase too.
>>>
>>> One side note regarding the gitlab shared runner:
>>>
>>> It seems the shared runner Windows VM is quite slow. Is it possible to
>>> get a faster VM externally?
>>
>> Any further comment for this patch?
>>
> 
> Ping?

As long as the Windows CI runners are still that slow, I'm still not 
convinced that it is a good idea to add more stuff here. Maybe to the 32-bit 
build, since it runs a bit faster, but I think we really should avoid to 
extend the 64-bit build.

  Thomas

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Thomas Huth]

On 29/10/2022 15.45, Bin Meng wrote:
> Hi Thomas,
> 
> On Wed, Sep 21, 2022 at 8:24 PM Thomas Huth <thuth@redhat.com> wrote:
>>
>> On 21/09/2022 14.18, Bin Meng wrote:
>>> Hi,
>>>
>>> On Thu, Sep 8, 2022 at 9:28 PM Bin Meng <bmeng.cn@gmail.com> wrote:
>>>>
>>>> At present packaging the required DLLs of QEMU executables is a
>>>> manual process, and error prone.
>>>>
>>>> Improve scripts/nsis.py by adding a logic to automatically package
>>>> required DLLs of QEMU executables.
>>>>
>>>> 'make installer' is tested in the cross-build on Linux in CI, but
>>>> not in the Windows native build. Update CI to test the installer
>>>> generation on Windows too.
>>>>
>>>> During testing a 32-bit build issue was exposed in block/nfs.c and
>>>> the fix is included in this series.
>>>>
>>>>
>>>> Bin Meng (7):
>>>>     scripts/nsis.py: Drop the unnecessary path separator
>>>>     scripts/nsis.py: Fix destination directory name when invoked on
>>>>       Windows
>>>>     scripts/nsis.py: Automatically package required DLLs of QEMU
>>>>       executables
>>>>     .gitlab-ci.d/windows.yml: Drop the sed processing in the 64-bit build
>>>>     block/nfs: Fix 32-bit Windows build
>>>>     .gitlab-ci.d/windows.yml: Unify the prerequisite packages
>>>>     .gitlab-ci.d/windows.yml: Test 'make installer' in the CI
>>>>
>>>>    meson.build              |  1 +
>>>>    block/nfs.c              |  8 ++++++
>>>>    .gitlab-ci.d/windows.yml | 40 ++++++++++++++++++++-------
>>>>    scripts/nsis.py          | 60 +++++++++++++++++++++++++++++++++-------
>>>>    4 files changed, 89 insertions(+), 20 deletions(-)
>>>>
>>>
>>> I see Thomas only queued patch #4 (".gitlab-ci.d/windows.yml: Drop the
>>> sed processing in the 64-bit build")
>>>
>>> What about other patches?
>>
>> I hope that Stefan Weil (our W32 maintainer) could have a look at these first...
>>
> 
> Stefan has reviewed / tested patch 1-3. Not sure who is going to queue
> these 3 patches?

If Stefan has time for a pull request, I think he would be the best fit. Stefan?

Otherwise, maybe Marc-André could take those patches, since he apparently 
wrote that nsis.py script?

(By the way, should we have an entry for that script in MAINTAINERS? ... 
likely in the W32/W64 section?)

  Thomas

Re: [PATCH 7/7] .gitlab-ci.d/windows.yml: Test 'make installer' in the CI

[Thomas Huth]

On 30/10/2022 04.21, Bin Meng wrote:
> On Sun, Oct 30, 2022 at 12:39 AM Stefan Weil <sw@weilnetz.de> wrote:
...
>> Maybe it is sufficient to build only a 64 bit installer. Is there still
>> need for QEMU on 32 bit Windows? For CI, most parts of the NSIS process
>> (which requires a lot of resources) are covered by either 32 or 64 bit
>> builds, so running both might be unnecessary.
> 
> I see no need to support QEMU on 32-bit Windows as it is less common.

If you feel confident that QEMU on 32-bit Windows is not worth to support 
anymore, could you please send a patch for docs/about/build-platforms.rst to 
state there that only 64-bit Windows versions are supported?

We could then also drop the 32-bit CI Windows jobs to save some precious CI 
minutes (since they are very limited nowadays on gitlab).

  Thomas

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Stefan Weil via]

Am 31.10.22 um 07:52 schrieb Thomas Huth:

> On 29/10/2022 15.45, Bin Meng wrote:
>> Stefan has reviewed / tested patch 1-3. Not sure who is going to queue
>> these 3 patches?
>
> If Stefan has time for a pull request, I think he would be the best 
> fit. Stefan?
>
> Otherwise, maybe Marc-André could take those patches, since he 
> apparently wrote that nsis.py script?
>
> (By the way, should we have an entry for that script in MAINTAINERS? 
> ... likely in the W32/W64 section?)
>
>  Thomas


Thanks. I have sent a pull request now.

Please excuse that some of you got that pull request e-mails twice.

I used Peter's make-pullreq script for the first time and had to learn 
how it works.

Stefan

Re: [PATCH 0/7] nsis: gitlab-ci: Improve QEMU Windows installer packaging

[Marc-André Lureau]

Hi Stefan

On Mon, Oct 31, 2022 at 1:27 PM Stefan Weil via <qemu-devel@nongnu.org> wrote:
>
> Am 31.10.22 um 07:52 schrieb Thomas Huth:
>
> > On 29/10/2022 15.45, Bin Meng wrote:
> >> Stefan has reviewed / tested patch 1-3. Not sure who is going to queue
> >> these 3 patches?
> >
> > If Stefan has time for a pull request, I think he would be the best
> > fit. Stefan?
> >
> > Otherwise, maybe Marc-André could take those patches, since he
> > apparently wrote that nsis.py script?
> >
> > (By the way, should we have an entry for that script in MAINTAINERS?
> > ... likely in the W32/W64 section?)
> >
> >  Thomas
>
>
> Thanks. I have sent a pull request now.
>
> Please excuse that some of you got that pull request e-mails twice.
>
> I used Peter's make-pullreq script for the first time and had to learn
> how it works.

Could you send a MAINTAINERS patch to add scripts/nsis.py to w32/w64?
thanks

-- 
Marc-André Lureau

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Stefan Weil via]

Am 10.09.22 um 02:37 schrieb Bin Meng:
> On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
> <mark.cave-ayland@ilande.co.uk> wrote:
>>
>> On 08/09/2022 14:28, Bin Meng wrote:
>>
>>> From: Bin Meng <bin.meng@windriver.com>
>>>
>>> At present packaging the required DLLs of QEMU executables is a
>>> manual process, and error prone.
>>>
>>> Actually build/config-host.mak contains a GLIB_BINDIR variable
>>> which is the directory where glib and other DLLs reside. This
>>> works for both Windows native build and cross-build on Linux.
>>> We can use it as the search directory for DLLs and automate
>>> the whole DLL packaging process.
>>>
>>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
>>> ---
>>>
>>>    meson.build     |  1 +
>>>    scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
>>>    2 files changed, 43 insertions(+), 4 deletions(-)
>>>
[...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py
>>> index baa6ef9594..03ed7608a2 100644
>>> --- a/scripts/nsis.py
>>> +++ b/scripts/nsis.py
>>> @@ -18,12 +18,36 @@ def signcode(path):
>>>            return
>>>        subprocess.run([cmd, path])
>>>
>>> +def find_deps(exe_or_dll, search_path, analyzed_deps):
>>> +    deps = [exe_or_dll]
>>> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)

This fails on non x86 hosts where objdump does not know how to handle a 
Windows x86_64 exe file.

>>> +    output = output.split("\n")
>>> +    for line in output:
>>> +        if not line.startswith("\tDLL Name: "):
>>> +            continue
>>> +
>>> +        dep = line.split("DLL Name: ")[1].strip()
>>> +        if dep in analyzed_deps:
>>> +            continue
>>> +
>>> +        dll = os.path.join(search_path, dep)
>>> +        if not os.path.exists(dll):
>>> +            # assume it's a Windows provided dll, skip it
>>> +            continue
>>> +
>>> +        analyzed_deps.add(dep)
>>> +        # locate the dll dependencies recursively
>>> +        rdeps = find_deps(dll, search_path, analyzed_deps)
>>> +        deps.extend(rdeps)
>>> +
>>> +    return deps
[...]
>>
>> FWIW I wrote a similar script a while back to help package a custom Windows build for
>> a client, however I used ldd instead of objdump since it provided the full paths for
>> DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the
>> QEMU build tree.
>>
> 
> Yep, ldd also works, but only on Windows native build. objdump can
> work on both Windows native and Linux cross builds.


objdump fails on Linux cross builds on any non x86 host, because objdump 
typically only supports the native host architecture.

Therefore I get an error on an ARM64 host (podman on Mac M1):

         objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file 
format not recognized

I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then 
native builds might fail because they don't have x86_64-w64-mingw32-objdump.

Is there a simple way how we can get the information here whether 
objdump requires a cross build prefix?

Stefan

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Bin Meng]

On Mon, Feb 26, 2024 at 1:37 AM Stefan Weil <sw@weilnetz.de> wrote:
>
> Am 10.09.22 um 02:37 schrieb Bin Meng:
> > On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
> > <mark.cave-ayland@ilande.co.uk> wrote:
> >>
> >> On 08/09/2022 14:28, Bin Meng wrote:
> >>
> >>> From: Bin Meng <bin.meng@windriver.com>
> >>>
> >>> At present packaging the required DLLs of QEMU executables is a
> >>> manual process, and error prone.
> >>>
> >>> Actually build/config-host.mak contains a GLIB_BINDIR variable
> >>> which is the directory where glib and other DLLs reside. This
> >>> works for both Windows native build and cross-build on Linux.
> >>> We can use it as the search directory for DLLs and automate
> >>> the whole DLL packaging process.
> >>>
> >>> Signed-off-by: Bin Meng <bin.meng@windriver.com>
> >>> ---
> >>>
> >>>    meson.build     |  1 +
> >>>    scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
> >>>    2 files changed, 43 insertions(+), 4 deletions(-)
> >>>
> [...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py
> >>> index baa6ef9594..03ed7608a2 100644
> >>> --- a/scripts/nsis.py
> >>> +++ b/scripts/nsis.py
> >>> @@ -18,12 +18,36 @@ def signcode(path):
> >>>            return
> >>>        subprocess.run([cmd, path])
> >>>
> >>> +def find_deps(exe_or_dll, search_path, analyzed_deps):
> >>> +    deps = [exe_or_dll]
> >>> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
>
> This fails on non x86 hosts where objdump does not know how to handle a
> Windows x86_64 exe file.

Does this command work in the MSYS2 environment on Windows Arm?

>
> >>> +    output = output.split("\n")
> >>> +    for line in output:
> >>> +        if not line.startswith("\tDLL Name: "):
> >>> +            continue
> >>> +
> >>> +        dep = line.split("DLL Name: ")[1].strip()
> >>> +        if dep in analyzed_deps:
> >>> +            continue
> >>> +
> >>> +        dll = os.path.join(search_path, dep)
> >>> +        if not os.path.exists(dll):
> >>> +            # assume it's a Windows provided dll, skip it
> >>> +            continue
> >>> +
> >>> +        analyzed_deps.add(dep)
> >>> +        # locate the dll dependencies recursively
> >>> +        rdeps = find_deps(dll, search_path, analyzed_deps)
> >>> +        deps.extend(rdeps)
> >>> +
> >>> +    return deps
> [...]
> >>
> >> FWIW I wrote a similar script a while back to help package a custom Windows build for
> >> a client, however I used ldd instead of objdump since it provided the full paths for
> >> DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the
> >> QEMU build tree.
> >>
> >
> > Yep, ldd also works, but only on Windows native build. objdump can
> > work on both Windows native and Linux cross builds.
>
>
> objdump fails on Linux cross builds on any non x86 host, because objdump
> typically only supports the native host architecture.
>
> Therefore I get an error on an ARM64 host (podman on Mac M1):
>
>          objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file
> format not recognized
>
> I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then
> native builds might fail because they don't have x86_64-w64-mingw32-objdump.
>
> Is there a simple way how we can get the information here whether
> objdump requires a cross build prefix?

For QEMU Windows, I believe the only supported architecture is x86_64,
correct? Do we want to support (cross) building QEMU for Windows Arm?

Based on your observation, objdump on Linux cross builds on any x86
host works, but not on non x86 host.
Maybe it's a hint to ask for binutils guys to include the PE format
support for objdump Arm by default.

Regards,
Bin

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Stefan Weil via]

[-- Attachment #1: Type: text/plain, Size: 4230 bytes --]

Am 26.02.24 um 05:35 schrieb Bin Meng:

> On Mon, Feb 26, 2024 at 1:37 AM Stefan Weil<sw@weilnetz.de>  wrote:
>> Am 10.09.22 um 02:37 schrieb Bin Meng:
>>> On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
>>> <mark.cave-ayland@ilande.co.uk>  wrote:
>>>> On 08/09/2022 14:28, Bin Meng wrote:
>>>>
>>>>> From: Bin Meng<bin.meng@windriver.com>
>>>>>
>>>>> At present packaging the required DLLs of QEMU executables is a
>>>>> manual process, and error prone.
>>>>>
>>>>> Actually build/config-host.mak contains a GLIB_BINDIR variable
>>>>> which is the directory where glib and other DLLs reside. This
>>>>> works for both Windows native build and cross-build on Linux.
>>>>> We can use it as the search directory for DLLs and automate
>>>>> the whole DLL packaging process.
>>>>>
>>>>> Signed-off-by: Bin Meng<bin.meng@windriver.com>
>>>>> ---
>>>>>
>>>>>     meson.build     |  1 +
>>>>>     scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
>>>>>     2 files changed, 43 insertions(+), 4 deletions(-)
>>>>>
>> [...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py
>>>>> index baa6ef9594..03ed7608a2 100644
>>>>> --- a/scripts/nsis.py
>>>>> +++ b/scripts/nsis.py
>>>>> @@ -18,12 +18,36 @@ def signcode(path):
>>>>>             return
>>>>>         subprocess.run([cmd, path])
>>>>>
>>>>> +def find_deps(exe_or_dll, search_path, analyzed_deps):
>>>>> +    deps = [exe_or_dll]
>>>>> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
>> This fails on non x86 hosts where objdump does not know how to handle a
>> Windows x86_64 exe file.
> Does this command work in the MSYS2 environment on Windows Arm?


I don't know and cannot test that, because I don't run Windows on ARM.

>>>>> +    output = output.split("\n")
>>>>> +    for line in output:
>>>>> +        if not line.startswith("\tDLL Name: "):
>>>>> +            continue
>>>>> +
>>>>> +        dep = line.split("DLL Name: ")[1].strip()
>>>>> +        if dep in analyzed_deps:
>>>>> +            continue
>>>>> +
>>>>> +        dll = os.path.join(search_path, dep)
>>>>> +        if not os.path.exists(dll):
>>>>> +            # assume it's a Windows provided dll, skip it
>>>>> +            continue
>>>>> +
>>>>> +        analyzed_deps.add(dep)
>>>>> +        # locate the dll dependencies recursively
>>>>> +        rdeps = find_deps(dll, search_path, analyzed_deps)
>>>>> +        deps.extend(rdeps)
>>>>> +
>>>>> +    return deps
>> [...]
>>>> FWIW I wrote a similar script a while back to help package a custom Windows build for
>>>> a client, however I used ldd instead of objdump since it provided the full paths for
>>>> DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the
>>>> QEMU build tree.
>>>>
>>> Yep, ldd also works, but only on Windows native build. objdump can
>>> work on both Windows native and Linux cross builds.
>>
>> objdump fails on Linux cross builds on any non x86 host, because objdump
>> typically only supports the native host architecture.
>>
>> Therefore I get an error on an ARM64 host (podman on Mac M1):
>>
>>           objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file
>> format not recognized
>>
>> I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then
>> native builds might fail because they don't have x86_64-w64-mingw32-objdump.
>>
>> Is there a simple way how we can get the information here whether
>> objdump requires a cross build prefix?
> For QEMU Windows, I believe the only supported architecture is x86_64,
> correct? Do we want to support (cross) building QEMU for Windows Arm?


Yes, I think we only support QEMU on Windows x86_64. I also don't know 
anyone who has tried building for Windows ARM. And up to now I also was 
never asked for that, so obviously there is still no need for it.


> Based on your observation, objdump on Linux cross builds on any x86
> host works, but not on non x86 host.
> Maybe it's a hint to ask for binutils guys to include the PE format
> support for objdump Arm by default.


I am afraid that we'll have to find a solution on the QEMU side, not 
wait until all binutils support the PE format for x86_64 (which would 
make the binaries or the library much larger).

Stefan


[-- Attachment #2: Type: text/html, Size: 6541 bytes --]

Re: [PATCH 3/7] scripts/nsis.py: Automatically package required DLLs of QEMU executables

[Mark Cave-Ayland]

On 26/02/2024 06:30, Stefan Weil via wrote:

> Am 26.02.24 um 05:35 schrieb Bin Meng:
> 
>> On Mon, Feb 26, 2024 at 1:37 AM Stefan Weil<sw@weilnetz.de>  wrote:
>>> Am 10.09.22 um 02:37 schrieb Bin Meng:
>>>> On Sat, Sep 10, 2022 at 12:49 AM Mark Cave-Ayland
>>>> <mark.cave-ayland@ilande.co.uk>  wrote:
>>>>> On 08/09/2022 14:28, Bin Meng wrote:
>>>>>
>>>>>> From: Bin Meng<bin.meng@windriver.com>
>>>>>>
>>>>>> At present packaging the required DLLs of QEMU executables is a
>>>>>> manual process, and error prone.
>>>>>>
>>>>>> Actually build/config-host.mak contains a GLIB_BINDIR variable
>>>>>> which is the directory where glib and other DLLs reside. This
>>>>>> works for both Windows native build and cross-build on Linux.
>>>>>> We can use it as the search directory for DLLs and automate
>>>>>> the whole DLL packaging process.
>>>>>>
>>>>>> Signed-off-by: Bin Meng<bin.meng@windriver.com>
>>>>>> ---
>>>>>>
>>>>>>     meson.build     |  1 +
>>>>>>     scripts/nsis.py | 46 ++++++++++++++++++++++++++++++++++++++++++----
>>>>>>     2 files changed, 43 insertions(+), 4 deletions(-)
>>>>>>
>>> [...]>>> diff --git a/scripts/nsis.py b/scripts/nsis.py
>>>>>> index baa6ef9594..03ed7608a2 100644
>>>>>> --- a/scripts/nsis.py
>>>>>> +++ b/scripts/nsis.py
>>>>>> @@ -18,12 +18,36 @@ def signcode(path):
>>>>>>             return
>>>>>>         subprocess.run([cmd, path])
>>>>>>
>>>>>> +def find_deps(exe_or_dll, search_path, analyzed_deps):
>>>>>> +    deps = [exe_or_dll]
>>>>>> +    output = subprocess.check_output(["objdump", "-p", exe_or_dll], text=True)
>>> This fails on non x86 hosts where objdump does not know how to handle a
>>> Windows x86_64 exe file.
>> Does this command work in the MSYS2 environment on Windows Arm?
> 
> 
> I don't know and cannot test that, because I don't run Windows on ARM.
> 
>>>>>> +    output = output.split("\n")
>>>>>> +    for line in output:
>>>>>> +        if not line.startswith("\tDLL Name: "):
>>>>>> +            continue
>>>>>> +
>>>>>> +        dep = line.split("DLL Name: ")[1].strip()
>>>>>> +        if dep in analyzed_deps:
>>>>>> +            continue
>>>>>> +
>>>>>> +        dll = os.path.join(search_path, dep)
>>>>>> +        if not os.path.exists(dll):
>>>>>> +            # assume it's a Windows provided dll, skip it
>>>>>> +            continue
>>>>>> +
>>>>>> +        analyzed_deps.add(dep)
>>>>>> +        # locate the dll dependencies recursively
>>>>>> +        rdeps = find_deps(dll, search_path, analyzed_deps)
>>>>>> +        deps.extend(rdeps)
>>>>>> +
>>>>>> +    return deps
>>> [...]
>>>>> FWIW I wrote a similar script a while back to help package a custom Windows build for
>>>>> a client, however I used ldd instead of objdump since it provided the full paths for
>>>>> DLLs installed in the msys2/mingw-w64 environment via pacman which were outside the
>>>>> QEMU build tree.
>>>>>
>>>> Yep, ldd also works, but only on Windows native build. objdump can
>>>> work on both Windows native and Linux cross builds.
>>> objdump fails on Linux cross builds on any non x86 host, because objdump
>>> typically only supports the native host architecture.
>>>
>>> Therefore I get an error on an ARM64 host (podman on Mac M1):
>>>
>>>           objdump: /tmp/tmpvae5u0qm/qemu/qemu-system-aarch64.exe: file
>>> format not recognized
>>>
>>> I could use x86_64-w64-mingw32-objdump to fix the cross builds, but then
>>> native builds might fail because they don't have x86_64-w64-mingw32-objdump.
>>>
>>> Is there a simple way how we can get the information here whether
>>> objdump requires a cross build prefix?
>> For QEMU Windows, I believe the only supported architecture is x86_64,
>> correct? Do we want to support (cross) building QEMU for Windows Arm?
> 
> 
> Yes, I think we only support QEMU on Windows x86_64. I also don't know anyone who has 
> tried building for Windows ARM. And up to now I also was never asked for that, so 
> obviously there is still no need for it.
> 
> 
>> Based on your observation, objdump on Linux cross builds on any x86
>> host works, but not on non x86 host.
>> Maybe it's a hint to ask for binutils guys to include the PE format
>> support for objdump Arm by default.
> 
> 
> I am afraid that we'll have to find a solution on the QEMU side, not wait until all 
> binutils support the PE format for x86_64 (which would make the binaries or the 
> library much larger).

Another thought here: now that python is required to build QEMU, is it possible to 
use a python-based PE parser to extract the dependency information for both Windows 
native and cross builds? For example using something like 
https://github.com/erocarrera/pefile?


ATB,

Mark.