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

Corruption of image files during migration or optimization #2803

Open
arturvader opened this issue May 8, 2024 · 32 comments
Open

Corruption of image files during migration or optimization #2803

arturvader opened this issue May 8, 2024 · 32 comments
Labels
Bug Bounty: $100 Fix this issue to claim the $100 bounty. bug

Comments

@arturvader
Copy link

When the script is triggered, the file from the app/images/brand_logo.jpg folder is copied to dist/images/brand_logo.jpg. There should be no changes to the file but it changes and becomes unreadable.

In another project, when using "gulp-tinify": "^1.0.2" the same problem.

I checked with gulp version 4.0.2 installed and everything works correctly.

Node version:
20.13.0
npm version:
10.5.2
gulp --version
CLI version: 3.0.0
Local version: 5.0.0

package.json:
{
"name": "test",
"version": "1.0.0",
"main": "gulpfile.js",
"private": true,
"type": "module",
"browserslist": [
"last 2 version",
"not dead",
"not ie <= 11",
"iOS >= 12"
],
"scripts": {
"test": "echo "Error: no test specified" && exit 1"
},
"author": "",
"license": "ISC",
"devDependencies": {
"gulp": "^5.0.0",
"gulp-imagemin": "^9.1.0",
"gulp-newer": "^1.4.0"
}
}

gulpfile.js:
import gulp from 'gulp';
import imagemin from 'gulp-imagemin';
import newer from 'gulp-newer';

const app = 'app',
dist = 'dist';

const config = {
app : {
svg : app + '/images//*.svg',
img : app + '/images/
/.{jpg,jpeg,png,gif,webp}',
},
dist : {
svg : dist + '/images/**/
.svg',
img : dist + '/images//*',
all : dist + '/
/*',
},
}

// Images

export const images = gulp.series(
function(cb){
return gulp.src(config.app.svg, { base: 'app' })
.pipe(newer(config.dist.svg))
.pipe(imagemin())
.pipe(gulp.dest(dist))
cb();
},
);
export const imgtinify = () => {
return gulp.src(config.app.img, { base: 'app', encoding: false, buffer: false })
.pipe(newer(config.dist.img))
.pipe(gulp.dest(dist))
};

// Watch

export const startwatch = () => {
gulp.watch(config.app.img, gulp.series(
imgtinify,
));
gulp.watch(config.app.svg, gulp.series(images, ));
};

// Default

export default gulp.series(
gulp.parallel(
images,
imgtinify,
),
gulp.parallel(
startwatch,
),
);
Screenshot 2024-05-08 at 20 43 30

@yocontra
Copy link
Member

yocontra commented May 8, 2024

Add {encoding: false} to any gulp.src call where you are using binary files - see now docs and release blog for v5 for info.

@yocontra yocontra closed this as completed May 8, 2024
@arturvader
Copy link
Author

Add {encoding: false} to any gulp.src call where you are using binary files - see now docs and release blog for v5 for info.

Pay attention to the code that I provided as a sample. What you wrote is indicated in the code ( gulp.src(config.app.img, { base: 'app', encoding: false, buffer: false }) ) and it does not help.

@yocontra
Copy link
Member

yocontra commented May 8, 2024

Sorry, the code wasn't formatted in the issue very well and I was looking at the SVG logic. With encoding: false it shouldn't make any changes to the file at all.

Just going to repost your code simplified and formatted for reference:

import gulp from 'gulp'
import newer from 'gulp-newer'

const app = 'app'
const dist = 'dist'

const config = {
  app: {
    img: app + '/images//.{jpg,jpeg,png,gif,webp}'
  },
  dist: {
    img: dist + '/images//*'
  }
}
export const imgtinify = () =>
  gulp
    .src(config.app.img, {
      base: 'app',
      encoding: false,
      buffer: false
    })
    .pipe(newer(config.dist.img))
    .pipe(gulp.dest(dist))

export default gulp.series(imgtinify)

Are you able to reproduce this with the above code, and does removing gulp-newer also reproduce it? Could you provide an example of a before and after file so I can look at the binary?

@yocontra yocontra reopened this May 8, 2024
@arturvader
Copy link
Author

Sorry, the code wasn't formatted in the issue very well and I was looking at the SVG logic. With encoding: false it shouldn't make any changes to the file at all.

Just going to repost your code simplified and formatted for reference:

import gulp from 'gulp'
import newer from 'gulp-newer'

const app = 'app'
const dist = 'dist'

const config = {
  app: {
    img: app + '/images//.{jpg,jpeg,png,gif,webp}'
  },
  dist: {
    img: dist + '/images//*'
  }
}
export const imgtinify = () =>
  gulp
    .src(config.app.img, {
      base: 'app',
      encoding: false,
      buffer: false
    })
    .pipe(newer(config.dist.img))
    .pipe(gulp.dest(dist))

export default gulp.series(imgtinify)

Are you able to reproduce this with the above code, and does removing gulp-newer also reproduce it? Could you provide an example of a before and after file so I can look at the binary?

Original image:
brand_logo

Resulting file:
brand_logo

@arturvader
Copy link
Author

Sorry, the code wasn't formatted in the issue very well and I was looking at the SVG logic. With encoding: false it shouldn't make any changes to the file at all.

Just going to repost your code simplified and formatted for reference:

import gulp from 'gulp'
import newer from 'gulp-newer'

const app = 'app'
const dist = 'dist'

const config = {
  app: {
    img: app + '/images//.{jpg,jpeg,png,gif,webp}'
  },
  dist: {
    img: dist + '/images//*'
  }
}
export const imgtinify = () =>
  gulp
    .src(config.app.img, {
      base: 'app',
      encoding: false,
      buffer: false
    })
    .pipe(newer(config.dist.img))
    .pipe(gulp.dest(dist))

export default gulp.series(imgtinify)

Are you able to reproduce this with the above code, and does removing gulp-newer also reproduce it? Could you provide an example of a before and after file so I can look at the binary?

I checked the test assembly with the code you wrote and got the same result.

@phated
Copy link
Member

phated commented May 9, 2024

I don't have much input right now, but I'm noticing that the globs look very broken. Perhaps this isn't even picking up the files at all and the output is an older generated file

@arturvader
Copy link
Author

when I copied the code here some characters are not published (double *, /images/**/ )

@yocontra
Copy link
Member

yocontra commented May 9, 2024

To simplify further and fix the globs in the code, does this reproduce?

import gulp from 'gulp'

export const imgtinify = () =>
  gulp
    .src('app/images/**/*.{jpg,jpeg,png,gif,webp}', {
      base: 'app',
      encoding: false,
      buffer: false
    })
    .pipe(gulp.dest('dist'))

export default gulp.series(imgtinify)

Is it possible that you ran it without encoding: false, it produced broken images, then you added encoding: false but the new images didn't get written because gulp-newer is just looking at the mtime of the original file? Try clearing the dist folder out before reproducing again.

@arturvader
Copy link
Author

To simplify further and fix the globs in the code, does this reproduce?

import gulp from 'gulp'

export const imgtinify = () =>
  gulp
    .src('app/images/**/*.{jpg,jpeg,png,gif,webp}', {
      base: 'app',
      encoding: false,
      buffer: false
    })
    .pipe(gulp.dest('dist'))

export default gulp.series(imgtinify)

Is it possible that you ran it without encoding: false, it produced broken images, then you added encoding: false but the new images didn't get written because gulp-newer is just looking at the mtime of the original file? Try clearing the dist folder out before reproducing again.

The source file was made in Photoshop, I deleted all the files in dist before launching gulp. This is a test build, the problem was discovered in 2 new projects with a lot of image files and gulp version 5 was installed for the first time. In projects where the old version (previous) of gulp remains, everything works correctly.

@arturvader
Copy link
Author

Please try to create a test project with the settings that I specified. Maybe I made a mistake somewhere. I tried it on my MacBook Pro and on Linux (Raspberry PI 5). Sorry for my English, it's not my native language.

@yocontra yocontra added bug Bug Bounty: $100 Fix this issue to claim the $100 bounty. labels May 10, 2024
@Nezarik-Intmax
Copy link

Try to add removeBOM: false to src options

@mjot
Copy link

mjot commented May 13, 2024

I had the same problem with this simple task for copying font files:

async function fontsBuild(site) {
    let path = setPathsForSite(site);
    let fontsConfig = {
        dir: path.src + 'fonts/',
        src: path.src + 'fonts/*.{woff,woff2,eot,ttf,svg}',
        build: path.dst + 'assets/fonts/'
    };

    const directoryExists = await checkDirectory(fontsConfig.dir);
    if (directoryExists) {
        return gulp.src(fontsConfig.src)
            .pipe(newer(fontsConfig.build))
            .pipe(gulp.dest(fontsConfig.build));
    }
}

By adding encoding: false to gulp.src like mentioned from @yocontra its works again:

return gulp.src(fontsConfig.src, { encoding: false })

@arturvader
Copy link
Author

Try to add removeBOM: false to src options

I tried what you wrote, it didn't help.

@vkorytska
Copy link

This is probably related to gulpjs/vinyl-fs#351

@DuaelFr

This comment has been minimized.

@BlueAbdul

This comment has been minimized.

@yavorski

This comment has been minimized.

@arturvader

This comment has been minimized.

@rustscr1pt

This comment has been minimized.

@ernilambar
Copy link

I was having same issue of corrupted image. Adding following arguments in src() fixed the issue.

{
  buffer: true,
  removeBOM: false
}

Example:

return gulp.src( [ 'src/images/**/*.*' ], {
  buffer: true,
  removeBOM: false
  } )
  .pipe( gulp.dest( 'assets/images' ) );
} );

@arturvader
Copy link
Author

true

I confirm that after setting the buffer value to true (buffer: true), the file was copied without changes and working.

@BlueAbdul
Copy link

true

I confirm that after setting the buffer value to true (buffer: true), the file was copied without changes and working.

Yeah it fixes the problem, I don't know why my comment got minimized as I'm only trying to help...

@woody-li
Copy link

{ buffer: true, removeBOM: false }

It still don't working for font sources, such as gulp-iconfont.

@PRR24
Copy link

PRR24 commented May 25, 2024

According to the doc, buffer is true by default. It changing it affects the outcome, there is possibly another smaller defect.

@PRR24
Copy link

PRR24 commented May 26, 2024

I think I found the cause of the problem:
line 34 in write-stream.js is currently reading ... encoding.enc !== .... It should be ... codec.enc !== ... instead.
After making the change, all my scripts started to work correctly.

While looking around in the code, two additional things caught my eye:

  • optResolver used in write-stream is not in sync with the resolver in gulp.src, so the encoding used in stream writing differs from stream reading. As write-stream is calling stream reading functions, it feels bit scary.
  • Resolver.resolve is called with second argument (file), but the function has only a single argument.

Possibly these are there for a reason, but just wanted to point them out.

@KarelWintersky
Copy link

KarelWintersky commented May 30, 2024

Same problem.

Yeah, buffer: true solves problem for binary files, but this solution is absolutely not obvious.

The need for this line breaks many packages that don't know about it. Such as:

Is it possible to specify this parameter somewhere globally for all tasks?

@PRR24
Copy link

PRR24 commented May 30, 2024

@KarelWintersky You may try, if an in-place patch of
node_modules/vinyl-fs/lib/dest/write-contents/write-stream.js:34
as per above solves the problem for you.
If yes, this is a global fix.

@yocontra
Copy link
Member

yocontra commented May 30, 2024

I think I found the cause of the problem: line 34 in write-stream.js is currently reading ... encoding.enc !== .... It should be ... codec.enc !== ... instead. After making the change, all my scripts started to work correctly.

While looking around in the code, two additional things caught my eye:

* optResolver used in write-stream is not in sync with the resolver in _gulp.src_, so the encoding used in stream writing differs from stream reading. As write-stream is calling stream reading functions, it feels bit scary.

* Resolver.resolve is called with second argument (file), but the function has only a single argument.

Possibly these are there for a reason, but just wanted to point them out.

@PRR24 Can you submit a PR or open an issue on vinyl-fs? Thank you for looking deeper into this.

@keitetran
Copy link

Add {encoding: false} to any gulp.src call where you are using binary files - see now docs and release blog for v5 for info.

I was add this for copy excel file.

@gulpjs gulpjs deleted a comment from aditya305 Jun 4, 2024
Ko2doo pushed a commit to Template-Craft/gulp-nunjucks-starter-kit that referenced this issue Jun 10, 2024
thibault added a commit to MTES-MCT/envergo that referenced this issue Jul 2, 2024
@alex-lenk
Copy link

encoding: false

{
  buffer: true,
  removeBOM: false
}

Praise be to you, kind person. You have solved my problem. I was already considering switching to version 4.

@twolfson
Copy link

twolfson commented Jul 4, 2024

In gulp.spritesmith we had a comparable problem but the fix had to also include add a {encoding: false} on the .dest() as well

twolfson/gulp.spritesmith#159

Example of fixed Gulpfile.js:

var gulp = require('gulp');
var spritesmith = require('gulp.spritesmith');

gulp.task('sprite', function () {
  var spriteData = gulp.src('images/*.png', {encoding: false}).pipe(spritesmith({
    imgName: 'sprite.png',
    cssName: 'sprite.css'
  }));
  return spriteData.pipe(gulp.dest('path/to/output/', {encoding: false}));
});

EDIT: I've also opened a complaint about encoding here, gulpjs/vinyl-fs#355

@GeorgeFlorian
Copy link

The following code produces broken images, after decompressing:

import gulp from "gulp";
import gzip from "gulp-gzip";
import gulpIf from "gulp-if";
import path from "path";

// Define file type checks
const isTextFile = (file) => /\.(html|css|js)$/.test(path.extname(file.path));
const isBinaryFile = (file) =>
  /\.(png|jpg|jpeg|gif|svg|ico)$/.test(path.extname(file.path));

export function compress() {
  return (
    gulp
      .src("dist/**/*")
      // Gzip text files with default encoding
      .pipe(
        gulpIf(
          isTextFile,
          gzip({
            gzipOptions: { level: 9 },
            append: true,
          })
        )
      )
      // Gzip binary files with { encoding: false }
      .pipe(
        gulpIf(
          isBinaryFile,
          gzip({
            gzipOptions: { level: 9 },
            append: true,
            encoding: false,
            buffer: true,
            removeBOM: false,
          })
        )
      )
      .pipe(gulp.dest("data"))
  ); // Output to LittleFS data directory
}

gulp.task("compress", compress);

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Bug Bounty: $100 Fix this issue to claim the $100 bounty. bug
Projects
None yet
Development

No branches or pull requests