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.

Sign up for GitHub

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

Jump to bottom

Windows: support info.size on wide-character systems (a.k.a. the C++17 upgrade) #3943

Open
3 tasks done
zkz098 opened this issue Jan 13, 2024 · 3 comments
Open
3 tasks done

Windows: support info.size on wide-character systems (a.k.a. the C++17 upgrade) #3943

zkz098 opened this issue Jan 13, 2024 · 3 comments
Labels
enhancement ready-to-ship
Milestone
v0.34.0

Comments

@zkz098
Copy link

zkz098 commented Jan 13, 2024

Possible bug

Is this a possible bug in a feature of sharp, unrelated to installation?

  • Running npm install sharp completes without error.
  • Running node -e "require('sharp')" completes without error.

If you cannot confirm both of these, please open an installation issue instead.

Are you using the latest version of sharp?

  • I am using the latest version of sharp as reported by npm view sharp dist-tags.latest.

If you cannot confirm this, please upgrade to the latest version and try again before opening an issue.

If you are using another package which depends on a version of sharp that is not the latest, please open an issue against that package instead.

What is the output of running npx envinfo --binaries --system --npmPackages=sharp --npmGlobalPackages=sharp?

System:
    OS: Windows 11 10.0.22635
    CPU: (16) x64 AMD Ryzen 7 5800U with Radeon Graphics
    Memory: 5.94 GB / 15.31 GB
  Binaries:
    Node: 20.11.0 - C:\Program Files\nodejs\node.EXE
    Yarn: 1.22.19 - ~\AppData\Roaming\npm\yarn.CMD
    npm: 10.2.4 - C:\Program Files\nodejs\npm.CMD
    pnpm: 8.12.0 - ~\AppData\Local\pnpm\pnpm.CMD
  npmPackages:
    sharp: ^0.33.2 => 0.33.2

What are the steps to reproduce?

For any input image processed by the Sharp library using the toFile operation, when the output path contains non-ASCII characters, the info.size of the toFile operation will become undefined instead of being a numeric value.

What is the expected behaviour?

The info.size of the toFile function should be a numeric value.

Please provide a minimal, standalone code sample, without other dependencies, that demonstrates this problem

import sharp from "sharp"

const path1 = './images/out.png'
const path2 = './图像/out.png'

const image = sharp('./images/404.png')
image
    .toFile(path2)
    .then((info)=>{
        console.log(info.size)
    })

It runs well when using path1, but when using path2, info.size becomes undefined.

Please provide sample image(s) that help explain this problem

无标题6
@zkz098 zkz098 added the triage label Jan 13, 2024
@zkz098 zkz098 mentioned this issue Jan 13, 2024
Open
@lovell
Copy link
Owner

lovell commented Jan 13, 2024

Thanks for reporting, this appears to be due to the way that Windows exposes filesystem data to C code.

Here's the logic in sharp that reads the size of the output file:

sharp/src/pipeline.cc

Line 1295 in 0dcc7d5

if (STAT64_FUNCTION(baton->fileOut.data(), &st) == 0) {

For Windows, the STAT64_FUNCTION used is _stat64:

sharp/src/pipeline.cc

Line 25 in 0dcc7d5

#define STAT64_FUNCTION _stat64

For "wide-character" paths the docs suggest using _wstat64 instead, however this doesn't support multi-byte strings, and therefore neither approach will work all of the time.

https://learn.microsoft.com/en-us/cpp/c-runtime-library/reference/stat-functions

I think the best thing to do here is upgrade to using C++17 and take advantage of its new std::filesystem API. This would allow us to remove all the platform-specific STAT64_FUNCTION logic from sharp.

@lovell lovell added enhancement and removed triage labels Jan 13, 2024
@lovell lovell changed the title When the output path of the toFile method contains non-ASCII characters, info.size will become undefined Windows: support info.size on wide-character systems (a.k.a. the C++17 upgrade) Jan 13, 2024
@lovell
Copy link
Owner

lovell commented Jan 20, 2024 •
edited
Loading

I had a quick look at what might be required for this.

https://github.com/lovell/sharp/compare/upgrade-to-c++17
https://github.com/lovell/sharp/actions/runs/7594372372

  • Linux: Full C++17 support was added in gcc 9 (I tried a few linker flags to force it using gcc 8, but nothing seemed to work). This means ARM v6 and s390x prebuilt binaries will require upgrading from Debian buster (gcc 8.3.0) to bullseye (gcc 10.2.1), so the minimum glibc version will increase from 2.28 to 2.31.

  • macOS: Full C++17 support was added in 10.15, so the minimum version will increase from 10.13 to 10.15.

  • Windows: Tests that result in an attempt to call file_size on a directory are crashing with exit code 3221226505 rather than throwing an exception, which I guess might relate to some C++ runtime version mismatch (<experimental/filesystem> vs <filesystem>?).

In summary and on balance, the risks from upgrading currently outweigh the benefits it might bring.

@lovell lovell mentioned this issue Mar 18, 2024
Closed
@lovell
Copy link
Owner

lovell commented Nov 1, 2024

Having revisited this, it looks like Windows was crashing when trying to (recursively? concurrently?) calculate the size of a directory, so I've updated the logic to prevent that happening - see bbe60cd

@lovell lovell added this to the v0.34.0 milestone Nov 1, 2024
@lovell lovell mentioned this issue Nov 1, 2024
Open
12 tasks
lovell added a commit that referenced this issue Nov 4, 2024
@lovell
Support info.size on wide-char systems via upgrade to C++17 #3943
06b08bf
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
enhancement ready-to-ship
Projects
None yet
Milestone
v0.34.0
Development

No branches or pull requests
2 participants
@lovell @zkz098