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

fix(windows): fix installing non-ascii paths and make normalizeBuf generic #8608

Merged
merged 24 commits into from
Feb 1, 2024

Conversation

dylan-conway
Copy link
Member

What does this PR do?

Fixes installing packages with non-ascii paths.

Comment in libarchive.zig:

// TODO:
// Due to path separator replacement and other copies that happen internally, libarchive changes the
// storage type of paths on windows to wide character strings. Using `archive_entry_pathname` or `archive_entry_pathname_utf8`
// on an wide character string will return null if there are non-ascii characters.
// (this can be seen by installing @fastify/send, which has a path "@fastify\send\test\fixtures\snow ☃")
//
// Ideally, we find a way to tell libarchive to not convert the strings to wide characters and also to not
// replace path separators. We can do both of these with our own normalization and utf8/utf16 string conversion code.

Also makes normalizeBuf and other functions it uses generic, so they can take u8 or u16 slices. In the future we should update more of our string operations to be generic.

How did you verify your code works?

manually tested installing @fastify/send

// replace path separators. We can do both of these with our own normalization and utf8/utf16 string conversion code.
var pathname: bun.OSPathSliceZ = if (comptime Environment.isWindows) brk: {
const normalized = bun.path.normalizeBuf(
u16,
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yay
generic normalizeBuf

src/libarchive/libarchive.zig Outdated Show resolved Hide resolved
Copy link
Contributor

github-actions bot commented Feb 1, 2024

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Copy link
Contributor

github-actions bot commented Feb 1, 2024

Copy link
Contributor

github-actions bot commented Feb 1, 2024

❌🪟 @dylan-conway, there are 9 test regressions on Windows x86_64

  • test\bundler\cli.test.ts
  • test\bundler\esbuild\splitting.test.ts
  • test\cli\inspect\inspect.test.ts
  • test\cli\run\transpiler-cache.test.ts
  • test\js\bun\util\bun-file-windows.test.ts
  • test\js\node\process\process-args.test.js
  • test\js\third_party\postgres\postgres.test.ts
  • test\regression\issue\08095.test.ts
  • test\transpiler\template-literal.test.ts

Full Test Output

@dylan-conway dylan-conway merged commit 4f98336 into main Feb 1, 2024
1 check passed
@dylan-conway dylan-conway deleted the dylan/fix-libarchive-wide-char-bug branch February 1, 2024 06:44
ryoppippi pushed a commit to ryoppippi/bun that referenced this pull request Feb 2, 2024
…generic (oven-sh#8608)

* comptime type normalizeStringBuf

* delete

* revert

* revert revert

* revert

* remove unused

* remove unnecessary assert

* add comment

* remove normalize, need ../

* use Output.err

* update error message

* generic T suffix

* fix windows build

* more fix build

* more fix build

* mkdiratZ

* update test

* [autofix.ci] apply automated fixes

* update snapshot again

* fix merge

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Hanaasagi pushed a commit to Hanaasagi/bun that referenced this pull request Feb 3, 2024
…generic (oven-sh#8608)

* comptime type normalizeStringBuf

* delete

* revert

* revert revert

* revert

* remove unused

* remove unnecessary assert

* add comment

* remove normalize, need ../

* use Output.err

* update error message

* generic T suffix

* fix windows build

* more fix build

* more fix build

* mkdiratZ

* update test

* [autofix.ci] apply automated fixes

* update snapshot again

* fix merge

---------

Co-authored-by: autofix-ci[bot] <114827586+autofix-ci[bot]@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants