Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BUG: Incorrect binary files installed with Node.js 18.16.0 and later, Node.js 19.8.0 and later, and Node.js 20 #7

Closed
sounisi5011 opened this issue May 30, 2023 · 4 comments · Fixed by #9

Comments

@sounisi5011
Copy link
Collaborator

sounisi5011 commented May 30, 2023

Note
This issue is simply a bug report for everyone, including go-npm users, and there is currently no way to fix it. Sorry I can't help you.

When @go-task/go-npm is run on the following Node.js, the binary file extracted from the .zip file has incorrect contents:

  • Node.js 18: 18.16.0 or later
  • Node.js 19: 19.8.0 or later
  • Node.js 20: All versions

This bug causes binary installations to fail, primarily on Windows.
The following error occurs when the command is run.

$ task --version
This version of ...\task.exe is not compatible with the version of Windows you're running. Check your computer's system information and then contact the software publisher.

However, the cause is the decompression process of the .zip file.
The same problem will occur with the CLI that distributes macOS and UNIX binaries as .zip files.

This bug has now been reported: ZJONSSON/node-unzipper#271
But 2 months after the report, it still has not been fixed. We do not know when this will be fixed.

@sounisi5011
Copy link
Collaborator Author

sounisi5011 commented May 30, 2023

This bug can be fixed by replacing unzipper with one of the following libraries:

  • yauzl-promise

    Pros 👍:

    • Well maintained. Last updated 12 days ago.
    • Only the necessary files can be decompressed, so there is no need to create temporary files.
    • Even when unzipping huge ZIP files, it reads the files in chunks so it does not consume too much memory.

    Cons 👎:

    • Node.js 16 or higher is supported. May not work with older versions of Node.js.
    example code

    Warning
    These codes are just usage examples. They are problematic code with no error handling, performance considerations, or edge case assumptions.
    If you open a pull request, please DO NOT USE these examples as they are.

    // Example of code to create a temporary file of a ZIP archive for decompression
    const { createWriteStream } = require('fs');
    const { join } = require('path');
    const pipelineAsync = require('stream/promises').pipeline;
    
    const yauzl = require('yauzl-promise');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const outZIPFilename = join(opts.binPath, 'foo.zip');
      req
        .pipe(createWriteStream(outZIPFilename))
        .on('finish', async () => {
          try {
            const zip = await yauzl.open(outZIPFilename);
            try {
              for await (const entry of zip) {
                if (entry.filename === opts.binName) {
                  const outpath = join(opts.binPath, entry.filename);
                  await pipelineAsync(
                    await entry.openReadStream(),
                    createWriteStream(outpath),
                  );
                }
              }
              onSuccess();
            } finally {
              zip.close();
            }
          } catch (error) {
            onError(error);
          }
        })
        .on('error', onError);
    }
    // Example of code to extract to memory without creating a ZIP archive for decompression
    const { createWriteStream } = require('fs');
    const { join } = require('path');
    const pipelineAsync = require('stream/promises').pipeline;
    
    const yauzl = require('yauzl-promise');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const len = Number(req.response?.headers['content-length']);
      const buf = Buffer.allocUnsafe(Number(len));
      let readLen = 0;
      req.response?.on('data', chunk => {
        if (Buffer.isBuffer(chunk)) {
          chunk.copy(buf, readLen);
          readLen += chunk.byteLength;
        }
      });
      req.response?.on('close', async () => {
        try {
          const zip = await yauzl.fromBuffer(buf);
          try {
            for await (const entry of zip) {
              if (entry.filename === opts.binName) {
                const outpath = join(opts.binPath, entry.filename);
                await pipelineAsync(
                  await entry.openReadStream(),
                  createWriteStream(outpath),
                );
              }
            }
            onSuccess();
          } finally {
            zip.close();
          }
        } catch (error) {
          onError(error);
        }
      });
      req.response?.on('error', onError);
    }
  • adm-zip

    Pros 👍:

    • Well maintained. Last updated 5 months ago.
    • Only the necessary files can be decompressed, so there is no need to create temporary files.
    • 0 Dependencies!

    Cons 👎:

    • Many issues remain open.
    • Even when extracting a huge ZIP file, all of its contents are extracted into memory.
    example code

    Warning
    These codes are just usage examples. They are problematic code with no error handling, performance considerations, or edge case assumptions.
    If you open a pull request, please DO NOT USE these examples as they are.

    // Example of code to create a temporary file of a ZIP archive for decompression
    const { createWriteStream, writeFileSync } = require('fs');
    const { join } = require('path');
    
    const AdmZip = require('adm-zip');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const outZIPFilename = join(opts.binPath, 'foo.zip');
      req
        .pipe(createWriteStream(outZIPFilename))
        .on('finish', () => {
          try {
            const zip = new AdmZip(outZIPFilename);
            const entry = zip.getEntry(opts.binName);
            if (entry && !entry?.isDirectory) {
              writeFileSync(
                join(opts.binPath, opts.binName),
                entry.getData(),
                { mode: entry.header.fileAttr || 0o666 },
              );
            }
            onSuccess();
          } catch (error) {
            onError(error);
          }
        })
        .on('error', onError);
    }
    // Example of code to extract to memory without creating a ZIP archive for decompression
    const { writeFileSync } = require('fs');
    const { join } = require('path');
    
    const AdmZip = require('adm-zip');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const len = Number(req.response?.headers['content-length']);
      const buf = Buffer.allocUnsafe(Number(len));
      let readLen = 0;
      req.response?.on('data', chunk => {
        if (Buffer.isBuffer(chunk)) {
          chunk.copy(buf, readLen);
          readLen += chunk.byteLength;
        }
      });
      req.response?.on('close', () => {
        try {
          const zip = new AdmZip(buf);
          const entry = zip.getEntry(opts.binName);
          if (entry && !entry?.isDirectory) {
            writeFileSync(
              join(opts.binPath, opts.binName),
              entry.getData(),
              { mode: entry.header.fileAttr || 0o666 },
            );
          }
          onSuccess();
        } catch (error) {
          onError(error);
        }
      });
      req.response?.on('error', onError);
    }
  • unzip-stream

    Pros 👍:

    • Same API as unzipper, just replace require('unzipper') with require('unzip-stream') to fix it.

    Cons 👎:

    • It has not been maintained for a long time. Last updated 3 years ago.
    • Many issues remain open.
    example code
    // Example of code to extract to memory without creating a ZIP archive for decompression
    const unzipStream = require('unzip-stream');
    
    function unzip({ opts, req, onSuccess, onError }) {
      const unzip = unzipStream.Extract({ path: opts.binPath });
    
      unzip.on('error', onError);
      unzip.on('close', onSuccess);
    
      req.pipe(unzip);
    }
  • decompress

    Pros 👍:

    • This is famous.
    • Only the necessary files can be created.

    Cons 👎:

    • It has not been maintained for a long time. Last updated 3 years ago. It is dependent on yauzl, so it may not have been maintained for practically 5 years.
    • Even when extracting a huge ZIP file, all of its contents are extracted into memory.
    example code

    Warning
    These codes are just usage examples. They are problematic code with no error handling, performance considerations, or edge case assumptions.
    If you open a pull request, please DO NOT USE these examples as they are.

    // Example of code to create a temporary file of a ZIP archive for decompression
    const { createWriteStream } = require('fs');
    const { join } = require('path');
    
    const decompress = require('decompress');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const outZIPFilename = join(opts.binPath, 'foo.zip');
      req
        .pipe(createWriteStream(outZIPFilename))
        .on('finish', async () => {
          try {
            await decompress(outZIPFilename, opts.binPath, {
              filter: file => file.path === opts.binName,
            });
            onSuccess();
          } catch (error) {
            onError(error);
          }
        })
        .on('error', onError);
    }
    // Example of code to extract to memory without creating a ZIP archive for decompression
    const decompress = require('decompress');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const len = Number(req.response?.headers['content-length']);
      const buf = Buffer.allocUnsafe(Number(len));
      let readLen = 0;
      req.response?.on('data', chunk => {
        if (Buffer.isBuffer(chunk)) {
          chunk.copy(buf, readLen);
          readLen += chunk.byteLength;
        }
      });
      req.response?.on('close', async () => {
        try {
          await decompress(buf, opts.binPath, {
            filter: file => file.path === opts.binName,
          });
          onSuccess();
        } catch (error) {
          onError(error);
        }
      });
      req.response?.on('error', onError);
    }
  • extract-zip

    Pros 👍:

    • This is famous.
    • Very simple API.
    • Even when unzipping huge ZIP files, it reads the files in chunks so it does not consume too much memory.

    Cons 👎:

    • What it gets is the filename of the ZIP, so the downloaded ZIP data must be written to the file system.
    • It has not been maintained for a long time. Last updated 3 years ago. It is dependent on yauzl, so it may not have been maintained for practically 5 years.
    • Many issues remain open.
    example code

    Warning
    These codes are just usage examples. They are problematic code with no error handling, performance considerations, or edge case assumptions.
    If you open a pull request, please DO NOT USE these examples as they are.

    // Example of code to create a temporary file of a ZIP archive for decompression
    const { createWriteStream } = require('fs');
    const { join, resolve } = require('path');
    
    const extract = require('extract-zip');
    
    function assets_unzip({ opts, req, onSuccess, onError }) {
      const outZIPFilename = join(opts.binPath, 'foo.zip');
      req
        .pipe(createWriteStream(outZIPFilename))
        .on('finish', async () => {
          try {
            await extract(outZIPFilename, { dir: resolve(opts.binPath) })
            onSuccess();
          } catch (error) {
            onError(error);
          }
        })
        .on('error', onError);
    }
  • yauzl

    Pros 👍:

    • This is famous.
    • Even when unzipping huge ZIP files, it reads the files in chunks so it does not consume too much memory.

    Cons 👎:

    • It has not been maintained for a long time. Last updated 5 years ago.
    • Many issues remain open.

I can open a pull request that replaces unzipper, which library should I choose?
In my opinion, yauzl-promise1 or adm-zip would be good. I think the other libraries are no longer maintained.

I would prefer yauzl-promise because adm-zip has the problem of excessive memory consumption.
However, depending on the Node.js version range supported by go-npm, yauzl-promise may not work.

Footnotes

  1. However, if we choose yauzl-promise, there is a possibility that @go-task/go-npm will not work with older Node.js.

@andreynering
Copy link
Member

Hi @sounisi5011,

Between the two the you suggested, I like three things about adm-zip:

  • It seems to support Node < 16 according to what you said;
  • It has no dependencies. Given we bundle our code into a single .js to distribute, I think having a small / no dependencies lib counts a lot;
  • It has 1.8k stars, compared to just 10 of yauzl-promise, which suggest this other one is not too battle-tested.

@sounisi5011
Copy link
Collaborator Author

Thanks for your reply! Okay, I will start creating pull requests with adm-zip.

Just for reference, what is the version range of Node.js that go-npm should support?
I would like to actually test the pull request I created with those Node.js.

@andreynering
Copy link
Member

@sounisi5011 According to this table seems that 16 is the oldest maintained one, and I'd say that's our target.

The fact that adm-zip happens to work on versions before is a plus if someone happen to need/try that, so we don't need to officially support it.

The fact the this lib is dependency-free + is more used is enough reason to choose it, IMO.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
2 participants