From 33e5862df890cd9e10001fafab75bb038617e0b3 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Fri, 12 Feb 2021 11:48:57 +0000 Subject: [PATCH 1/4] backends/debian/debpkg: Forget a package's filename when removing its temporary dir When downloading remote packages, we place them into a package specific temporary directory. This is so we can remove them once we think we're done, so that peak space usage is minimised. We decide if a package is downloaded or not by the 'local' filename being set. If it's set, we return this path to callers and then they can try to operate on it. The problem is, we were failing to clear this variable when removing the temporary directory in some circumstances. That was meaning that a stale path could be given out to callers, a path to a file that doesn't exist any more. Fix this by always clearing the local path when removing the temporary directory. That means that the next time the file is requested it will be re-downloaded if this is a remote path. --- src/asgen/backends/debian/debpkg.d | 8 ++++++-- 1 file changed, 6 insertions(+), 2 deletions(-) diff --git a/src/asgen/backends/debian/debpkg.d b/src/asgen/backends/debian/debpkg.d index 469511e0..a36c8bff 100644 --- a/src/asgen/backends/debian/debpkg.d +++ b/src/asgen/backends/debian/debpkg.d @@ -327,7 +327,12 @@ public: dataArchive.close (); try { - if (std.file.exists (tmpDir)) + if (std.file.exists (tmpDir)) { + /* Whenever we delete the temporary directory, we need to + * forget about the local file too, since (if it's remote) that + * was downloaded into there. */ + logDebug ("Deleting temporary directory %s", tmpDir); + localDebFname = null; rmdirRecurse (tmpDir); } catch (Exception e) { // we ignore any error @@ -338,7 +343,6 @@ public: override final void finish () { - localDebFname = null; cleanupTemp (); } } From 188c18bd733fd2df83c51579ab54563e48be045b Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Fri, 12 Feb 2021 12:44:22 +0000 Subject: [PATCH 2/4] backends/debian/debpkg.d: Add more synchronization These operations are most thread unsafe; they are dealing with the underlying filesystem. Make sure they are synchronized. --- src/asgen/backends/debian/debpkg.d | 101 ++++++++++++++++------------- 1 file changed, 56 insertions(+), 45 deletions(-) diff --git a/src/asgen/backends/debian/debpkg.d b/src/asgen/backends/debian/debpkg.d index a36c8bff..ebe83966 100644 --- a/src/asgen/backends/debian/debpkg.d +++ b/src/asgen/backends/debian/debpkg.d @@ -174,21 +174,23 @@ public: { import std.regex : ctRegex; - if (dataArchive.isOpen) - return dataArchive; - - ArchiveDecompressor ad; - // extract the payload to a temporary location first - ad.open (this.getFilename); - mkdirRecurse (tmpDir); - - auto files = ad.extractFilesByRegex (ctRegex!(r"data\.*"), tmpDir); - if (files.length == 0) - throw new Exception ("Unable to find the payload tarball in Debian package: %s".format (this.getFilename)); - immutable dataArchiveFname = files[0]; - - dataArchive.open (dataArchiveFname); - return dataArchive; + synchronized(this) { + if (dataArchive.isOpen) + return dataArchive; + + ArchiveDecompressor ad; + // extract the payload to a temporary location first + ad.open (this.getFilename); + mkdirRecurse (tmpDir); + + auto files = ad.extractFilesByRegex (ctRegex!(r"data\.*"), tmpDir); + if (files.length == 0) + throw new Exception ("Unable to find the payload tarball in Debian package: %s".format (this.getFilename)); + immutable dataArchiveFname = files[0]; + + dataArchive.open (dataArchiveFname); + return dataArchive; + } } final void extractPackage (const string dest = buildPath (tmpDir, name)) @@ -196,32 +198,36 @@ public: import std.file : exists; import std.regex : ctRegex; - if (!dest.exists) - mkdirRecurse (dest); + synchronized(this) { + if (!dest.exists) + mkdirRecurse (dest); - auto pa = openPayloadArchive (); - pa.extractArchive (dest); + auto pa = openPayloadArchive (); + pa.extractArchive (dest); + } } private final auto openControlArchive () { import std.regex : ctRegex; - if (controlArchive.isOpen) - return controlArchive; + synchronized(this) { + if (controlArchive.isOpen) + return controlArchive; - ArchiveDecompressor ad; - // extract the payload to a temporary location first - ad.open (this.getFilename); - mkdirRecurse (tmpDir); + ArchiveDecompressor ad; + // extract the payload to a temporary location first + ad.open (this.getFilename); + mkdirRecurse (tmpDir); - auto files = ad.extractFilesByRegex (ctRegex!(r"control\.*"), tmpDir); - if (files.empty) - throw new Exception ("Unable to find control data in Debian package: %s".format (this.getFilename)); - immutable controlArchiveFname = files[0]; + auto files = ad.extractFilesByRegex (ctRegex!(r"control\.*"), tmpDir); + if (files.empty) + throw new Exception ("Unable to find control data in Debian package: %s".format (this.getFilename)); + immutable controlArchiveFname = files[0]; - controlArchive.open (controlArchiveFname); - return controlArchive; + controlArchive.open (controlArchiveFname); + return controlArchive; + } } override final @@ -323,20 +329,25 @@ public: override final void cleanupTemp () { - controlArchive.close (); - dataArchive.close (); - - try { - if (std.file.exists (tmpDir)) { - /* Whenever we delete the temporary directory, we need to - * forget about the local file too, since (if it's remote) that - * was downloaded into there. */ - logDebug ("Deleting temporary directory %s", tmpDir); - localDebFname = null; - rmdirRecurse (tmpDir); - } catch (Exception e) { - // we ignore any error - logDebug ("Unable to remove temporary directory: %s (%s)", tmpDir, e.msg); + synchronized(this) { + if (controlArchive.isOpen) + controlArchive.close (); + if (dataArchive.isOpen) + dataArchive.close (); + + try { + if (std.file.exists (tmpDir)) { + /* Whenever we delete the temporary directory, we need to + * forget about the local file too, since (if it's remote) that + * was downloaded into there. */ + logDebug ("Deleting temporary directory %s", tmpDir); + localDebFname = null; + rmdirRecurse (tmpDir); + } + } catch (Exception e) { + // we ignore any error + logDebug ("Unable to remove temporary directory: %s (%s)", tmpDir, e.msg); + } } } From 008a7d0c1568f23ecbdbd3579f3f748a4eacc69d Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Thu, 11 Feb 2021 10:38:40 +0000 Subject: [PATCH 3/4] zarchive: Log the error when we can't open an archive libarchive stores errno with the actual cause of the error. Print it. --- src/asgen/zarchive.d | 25 +++++++++++++++++++------ 1 file changed, 19 insertions(+), 6 deletions(-) diff --git a/src/asgen/zarchive.d b/src/asgen/zarchive.d index 6eeac96f..9381f356 100644 --- a/src/asgen/zarchive.d +++ b/src/asgen/zarchive.d @@ -19,6 +19,7 @@ module asgen.zarchive; +import core.stdc.string : strerror; import std.stdio; import std.string; import std.regex; @@ -97,8 +98,15 @@ string decompressFile (string fname) archive_read_support_filter_all (ar); ret = archive_read_open_filename (ar, toStringz (fname), DEFAULT_BLOCK_SIZE); - if (ret != ARCHIVE_OK) - throw new Exception (format ("Unable to open compressed file '%s': %s", fname, getArchiveErrorMessage (ar))); + if (ret != ARCHIVE_OK) { + auto ret_errno = archive_errno (ar); + auto ret_strerr = fromStringz (strerror(ret_errno)); + throw new Exception (format ("Unable to open compressed file '%s': %s. error: %s (%d)", + fname, + getArchiveErrorMessage (ar), + ret_strerr, + ret_errno)); + } return readArchiveData (ar, fname); } @@ -177,10 +185,15 @@ private: archive_read_support_format_all (ar); auto ret = archive_read_open_filename (ar, archive_fname.toStringz, DEFAULT_BLOCK_SIZE); - if (ret != ARCHIVE_OK) - throw new Exception (format ("Unable to open compressed file '%s': %s", - archive_fname, - getArchiveErrorMessage (ar))); + if (ret != ARCHIVE_OK) { + auto ret_errno = archive_errno (ar); + auto ret_strerr = fromStringz (strerror(ret_errno)); + throw new Exception (format ("Unable to open compressed file '%s': %s. error: %s (%d)", + archive_fname, + getArchiveErrorMessage (ar), + ret_strerr, + ret_errno)); + } return ar; } From 798272217e2b3130b403c0ae261a7c62a47086e1 Mon Sep 17 00:00:00 2001 From: Iain Lane Date: Fri, 12 Feb 2021 17:11:35 +0000 Subject: [PATCH 4/4] various: Improve builddir != srcdir builds and build the snap like this Newer versions of snapcraft seem to have changed the cwd of the build step. It's more robust to do this by absolute path anyway, so do that. A couple of parts of the build system want fixing for that- - Run the tests from the root of the source tree, and - Find data assets relative to the cwd Another alternative would be to do something like `G_TEST_{BUILD,SRC}DIR` that GLib has - environment variables that can be defined to help tests find their assets. But this works OK for now. Also run the sedding on the .gir file in override-pull; if the build step is run twice then this results in messed up files otherwise. --- snapcraft.yaml | 20 ++++++++++---------- src/asgen/meson.build | 4 +++- src/asgen/utils.d | 4 ++++ 3 files changed, 17 insertions(+), 11 deletions(-) diff --git a/snapcraft.yaml b/snapcraft.yaml index 6e25c7ff..4e0e0175 100644 --- a/snapcraft.yaml +++ b/snapcraft.yaml @@ -55,13 +55,18 @@ parts: appstream-generator: source: . source-type: git - parse-info: [data/org.freedesktop.appstream.generator.metainfo.xml] + parse-info: [usr/share/metainfo/org.freedesktop.appstream.generator.metainfo.xml] override-pull: | snapcraftctl pull # set version from Git snapcraftctl set-version "$(git describe --always | sed -e 's/v//;s/-/+git/;y/-/./')" + # adjust to an absolute path to help finding the GIR file from the AppStream part + sed -i 's|AppStream-1.0.gir|$SNAPCRAFT_STAGE/usr/share/gir-1.0/AppStream-1.0.gir|g' ${SNAPCRAFT_PART_SRC}/contrib/girwrap/APILookupAppStream.txt + sed -i 's|AppStreamCompose-1.0.gir|$SNAPCRAFT_STAGE/usr/share/gir-1.0/AppStreamCompose-1.0.gir|g' ${SNAPCRAFT_PART_SRC}/contrib/girwrap/APILookupAppStreamCompose.txt + + plugin: meson build-environment: - LD_LIBRARY_PATH: $SNAPCRAFT_STAGE/usr/lib/$SNAPCRAFT_ARCH_TRIPLET/ @@ -71,18 +76,13 @@ parts: echo "deb https://dl.yarnpkg.com/debian/ stable main" | sudo tee /etc/apt/sources.list.d/yarn.list apt-get update && apt-get install --no-install-recommends -y yarn nodejs - # adjust to an absolute path to help finding the GIR file from the AppStream part - sed -i 's|AppStream-1.0.gir|$SNAPCRAFT_STAGE/usr/share/gir-1.0/AppStream-1.0.gir|g' contrib/girwrap/APILookupAppStream.txt - sed -i 's|AppStreamCompose-1.0.gir|$SNAPCRAFT_STAGE/usr/share/gir-1.0/AppStreamCompose-1.0.gir|g' contrib/girwrap/APILookupAppStreamCompose.txt - # actually build asgen - we need to run everything manually here, # because snapcraft will kill the build if run with maximum amount of ninja jobs, # and I found no way to limit the amount of permitted ninja jobs other than overriding everything - meson --prefix=/usr --buildtype=debugoptimized -Ddownload-js=true snapbuild - cd snapbuild - ninja -j1 - meson test --verbose - DESTDIR=$SNAPCRAFT_PART_INSTALL ninja install + meson --prefix=/usr --buildtype=debugoptimized -Ddownload-js=true ${SNAPCRAFT_PART_BUILD} ${SNAPCRAFT_PART_SRC} + ninja -C "${SNAPCRAFT_PART_BUILD}" -j4 + meson test -C "${SNAPCRAFT_PART_BUILD}" --verbose + DESTDIR=${SNAPCRAFT_PART_INSTALL} ninja -C "${SNAPCRAFT_PART_BUILD}" install override-prime: | set -eux snapcraftctl prime diff --git a/src/asgen/meson.build b/src/asgen/meson.build index cde84364..42913dc6 100644 --- a/src/asgen/meson.build +++ b/src/asgen/meson.build @@ -110,4 +110,6 @@ asgen_test_exe = executable('asgen_test', d_import_dirs: [data_import_dirs], d_unittest: true ) -test('asgen_tests', asgen_test_exe) +test('asgen_tests', + asgen_test_exe, + workdir: meson.source_root()) diff --git a/src/asgen/utils.d b/src/asgen/utils.d index 615757f9..8cd2dd12 100644 --- a/src/asgen/utils.d +++ b/src/asgen/utils.d @@ -368,6 +368,10 @@ string getDataPath (string fname) if (std.file.exists (resPath)) return resPath; + resPath = buildPath ("data", fname); + if (std.file.exists (resPath)) + return resPath; + // Uh, let's just give up return buildPath ("/usr", "share", "appstream", fname); }