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(turbopack-node) postcss.config.js path resolution on Windows #7995

Merged
merged 4 commits into from
Apr 17, 2024

Conversation

bgw
Copy link
Member

@bgw bgw commented Apr 17, 2024

Description

Absolute paths don't work for imports on windows because c:\ is interpreted as a URI scheme. The workaround is to use an unambiguous file:// URI.

Fixes vercel/next.js#63755

Similar to vercel/next.js#64386

Testing Instructions

patch next.js's Cargo.toml to point to the updated branch
bgw@BENJAMINWOOCDA2 CLANGARM64 ~/Documents/next.js (canary)
$ git diff Cargo.toml
diff --git a/Cargo.toml b/Cargo.toml
index ffabdcc19..3ce91bc04 100644
--- a/Cargo.toml
+++ b/Cargo.toml
@@ -37,11 +37,11 @@ swc_core = { version = "0.90.30", features = [
 testing = { version = "0.35.22" }

 # Turbo crates
-turbopack-binding = { git = "https://github.com/vercel/turbo.git", tag = "turbopack-240416.1" }
+turbopack-binding = { git = "https://github.com/vercel/turbo.git", branch = "bgw/windows-postcss-config-path" }
 # [TODO]: need to refactor embed_directory! macro usages, as well as resolving turbo_tasks::function, macros..
-turbo-tasks = { git = "https://github.com/vercel/turbo.git", tag = "turbopack-240416.1" }
+turbo-tasks = { git = "https://github.com/vercel/turbo.git", branch = "bgw/windows-postcss-config-path" }
 # [TODO]: need to refactor embed_directory! macro usage in next-core
-turbo-tasks-fs = { git = "https://github.com/vercel/turbo.git", tag = "turbopack-240416.1" }
+turbo-tasks-fs = { git = "https://github.com/vercel/turbo.git", branch = "bgw/windows-postcss-config-path" }

 # General Deps
build the next-swc binary
cd next.js
turbo run build-native-no-plugin-woa-release
Run `next dev` in the test repo
cd ~/Documents/browserslist-file-resolution-turbo-win32
__INTERNAL_CUSTOM_TURBOPACK_BINDINGS=~/Documents/next.js/packages/next-swc/native/next-swc.win32-arm64-msvc.node node_modules/.bin/next dev --turbo

Screenshot 2024-04-16 at 5 37 19 PM

Closes PACK-2972

Copy link

vercel bot commented Apr 17, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
examples-nonmonorepo ✅ Ready (Inspect) Visit Preview Apr 17, 2024 0:47am
rust-docs ✅ Ready (Inspect) Visit Preview Apr 17, 2024 0:47am
8 Ignored Deployments
Name Status Preview Updated (UTC)
examples-basic-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-designsystem-docs ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-gatsby-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-kitchensink-blog ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-native-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-svelte-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-tailwind-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am
examples-vite-web ⬜️ Ignored (Inspect) Apr 17, 2024 0:47am

Copy link
Contributor

🟢 Turbopack Benchmark CI successful 🟢

Thanks

Copy link
Contributor

✅ This change can build next-swc

Copy link
Contributor

github-actions bot commented Apr 17, 2024

⚠️ CI failed ⚠️

The following steps have failed in CI:

  • Turbopack Rust tests (mac/win, non-blocking)

See workflow summary for details

@bgw
Copy link
Member Author

bgw commented Apr 17, 2024

There's a CI failure on Windows:

Caused by:
  for `node-file-trace`, command `'D:\a\turbo\turbo\target\release\deps\node_file_trace-68c90777c206c864.exe' --list --format terse` exited with code 0xc0000142: A dynamic link library (DLL) initialization routine failed. (os error 1114)

However, other pull requests appear to be failing with this same error, so I don't think this is related to my PR. I'm going to go ahead and merge.

@bgw bgw merged commit e7a5c60 into main Apr 17, 2024
48 of 50 checks passed
@bgw bgw deleted the bgw/windows-postcss-config-path branch April 17, 2024 02:13

const mod = await __turbopack_external_import__(configPath);
const configPath = `${{process.cwd()}}/${{{config_path}}}`;
Copy link
Member

Choose a reason for hiding this comment

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

This would better use path.join since the / is technically not the correct path separator on windows.

Copy link
Member Author

@bgw bgw Apr 19, 2024

Choose a reason for hiding this comment

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

@sokra I was trying to keep the change small, and I saw in the node documentation that:

On Windows, both the forward slash (/) and backward slash () are accepted as path segment separators; however, the path methods only add backward slashes ().

If you'd like, I can submit a follow-up PR.

Edit: #8105

bgw added a commit that referenced this pull request Apr 17, 2024
Absolute paths don't work for imports on windows because `c:\` is
interpreted as a URI scheme. The workaround is to use an unambiguous
`file://` URI.

Fixes vercel/next.js#63755

Similar to vercel/next.js#64386
bgw added a commit to vercel/next.js that referenced this pull request Apr 17, 2024
…ution on Windows

* vercel/turborepo#7995 <!-- Benjamin Woodruff - fix(turbopack-node) postcss.config.js path resolution on Windows  -->
ztanner pushed a commit to vercel/next.js that referenced this pull request Apr 17, 2024
…ution on Windows (#64677)

Not 100% sure I did this process right for a hotfix, but this new tag
should be exactly the same as what's currently in the 14-2-1 branch,
except with this one PR:

* vercel/turborepo#7995 <!-- Benjamin Woodruff -
fix(turbopack-node) postcss.config.js path resolution on Windows -->

---

In turbo, I ran

```
git switch --detach turbopack-240411.3
git switch -c bgw/hotfix-turbopack-14-2-1
git cherry-pick e7a5c60a3b899d0c4293589c2844016da82442f7
git push origin HEAD
```

I then ran the GitHub action for the nightly release process for that
new branch:
https://github.com/vercel/turbo/actions/workflows/turbopack-nightly-release.yml
(it seems like this just cuts a tag, I wonder for hotfixes if we should
manually cut the tag so it's named differently)

In the nextpack repository:

```
pnpm next-link turbopack-240417.2
```
bgw added a commit to vercel/next.js that referenced this pull request Apr 17, 2024
* vercel/turborepo#7995 <!-- Benjamin Woodruff - fix(turbopack-node) postcss.config.js path resolution on Windows  -->

(plus unrelated turborepo + CI changes)
bgw added a commit to vercel/next.js that referenced this pull request Apr 17, 2024
* vercel/turborepo#7995 <!-- Benjamin Woodruff - fix(turbopack-node) postcss.config.js path resolution on Windows  -->

(plus unrelated turborepo + CI changes)

This PR should not be cherrypicked into the 14-2-1 branch. Instead #64677 should be merged into that branch, as that PR contains only the one above Windows commit.
bgw added a commit that referenced this pull request May 7, 2024
sokra pushed a commit that referenced this pull request May 8, 2024
### Description

A followup requested by @sokra in
#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for #7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
Neosoulink pushed a commit to Neosoulink/turbo that referenced this pull request Jun 14, 2024
### Description

A followup requested by @sokra in
vercel#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for vercel#7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
…cel/turborepo#7995)

Absolute paths don't work for imports on windows because `c:\` is
interpreted as a URI scheme. The workaround is to use an unambiguous
`file://` URI.

Fixes #63755

Similar to #64386
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 25, 2024
### Description

A followup requested by @sokra in
vercel/turborepo#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for #7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
…cel/turborepo#7995)

Absolute paths don't work for imports on windows because `c:\` is
interpreted as a URI scheme. The workaround is to use an unambiguous
`file://` URI.

Fixes #63755

Similar to #64386
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

A followup requested by @sokra in
vercel/turborepo#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for #7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
…cel/turborepo#7995)

Absolute paths don't work for imports on windows because `c:\` is
interpreted as a URI scheme. The workaround is to use an unambiguous
`file://` URI.

Fixes #63755

Similar to #64386
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Jul 29, 2024
### Description

A followup requested by @sokra in
vercel/turborepo#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for #7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
…cel/turborepo#7995)

Absolute paths don't work for imports on windows because `c:\` is
interpreted as a URI scheme. The workaround is to use an unambiguous
`file://` URI.

Fixes #63755

Similar to #64386
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 1, 2024
### Description

A followup requested by @sokra in
vercel/turborepo#7995 (comment)

There's no meaningful change in behavior here, as node on windows
interprets both `/` and `\` as valid path separators, but this makes the
intent clearer.

### Testing Instructions

<details><summary>Repeated the test plan for #7995. Tested on both
Windows and Linux.</summary>

![Screenshot 2024-05-07 at
2.22.17 PM.png](https://graphite-user-uploaded-assets-prod.s3.amazonaws.com/HAZVitxRNnZz8QMiPn4a/bf7afcfb-0834-454a-b5ee-f52ae736eb47.png)
</details>


Closes PACK-3049
ForsakenHarmony pushed a commit to vercel/next.js that referenced this pull request Aug 16, 2024
* vercel/turborepo#7995 <!-- Benjamin Woodruff - fix(turbopack-node) postcss.config.js path resolution on Windows  -->

(plus unrelated turborepo + CI changes)

This PR should not be cherrypicked into the 14-2-1 branch. Instead #64677 should be merged into that branch, as that PR contains only the one above Windows commit.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

[TurboPack] When using a custom PostCSS config, TurboPack fails to compile application on Windows
3 participants