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

Reapply "validate path lengths before writes and attempt to eagerly c… #69569

Merged
merged 3 commits into from
Sep 10, 2024

Conversation

arlyon
Copy link
Contributor

@arlyon arlyon commented Sep 2, 2024

Reland of #69141 after it got reverted. Turns out there was a GT submit that got lost (along with the associated commit) somehow.

@ijjk ijjk added the created-by: Turbopack team PRs by the Turbopack team. label Sep 2, 2024
@arlyon arlyon force-pushed the arlyon/attempt-fix-too-long-file branch 3 times, most recently from fa35e58 to 7890120 Compare September 2, 2024 10:57
@ijjk
Copy link
Member

ijjk commented Sep 2, 2024

Tests Passed

@ijjk
Copy link
Member

ijjk commented Sep 2, 2024

Stats from current PR

Default Build
General
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
buildDuration 17.5s 19s ⚠️ +1.5s
buildDurationCached 8.5s 7.2s N/A
nodeModulesSize 358 MB 357 MB N/A
nextStartRea..uration (ms) 443ms 433ms N/A
Client Bundles (main, webpack)
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
1062.HASH.js gzip 169 B 168 B N/A
2cd9da90-HASH.js gzip 52 kB 52 kB N/A
5867-HASH.js gzip 42.5 kB 42.4 kB N/A
8503-HASH.js gzip 5.25 kB 5.25 kB N/A
framework-HASH.js gzip 56.9 kB 56.9 kB N/A
main-app-HASH.js gzip 224 B 225 B N/A
main-HASH.js gzip 32.6 kB 32.5 kB N/A
webpack-HASH.js gzip 1.71 kB 1.71 kB N/A
Overall change 0 B 0 B
Legacy Client Bundles (polyfills)
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
polyfills-HASH.js gzip 31 kB 31 kB
Overall change 31 kB 31 kB
Client Pages
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
_app-HASH.js gzip 193 B 193 B
_error-HASH.js gzip 191 B 191 B
amp-HASH.js gzip 511 B 512 B N/A
css-HASH.js gzip 342 B 343 B N/A
dynamic-HASH.js gzip 1.84 kB 1.84 kB N/A
edge-ssr-HASH.js gzip 265 B 266 B N/A
head-HASH.js gzip 362 B 364 B N/A
hooks-HASH.js gzip 392 B 392 B
image-HASH.js gzip 4.4 kB 4.4 kB N/A
index-HASH.js gzip 268 B 267 B N/A
link-HASH.js gzip 2.81 kB 2.81 kB N/A
routerDirect..HASH.js gzip 329 B 327 B N/A
script-HASH.js gzip 398 B 396 B N/A
withRouter-HASH.js gzip 325 B 324 B N/A
1afbb74e6ecf..834.css gzip 106 B 106 B
Overall change 882 B 882 B
Client Build Manifests
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
_buildManifest.js gzip 749 B 748 B N/A
Overall change 0 B 0 B
Rendered Page Sizes
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
index.html gzip 521 B 522 B N/A
link.html gzip 535 B 536 B N/A
withRouter.html gzip 518 B 518 B
Overall change 518 B 518 B
Edge SSR bundle Size
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
edge-ssr.js gzip 128 kB 128 kB N/A
page.js gzip 180 kB 178 kB N/A
Overall change 0 B 0 B
Middleware size
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
middleware-b..fest.js gzip 670 B 668 B N/A
middleware-r..fest.js gzip 156 B 157 B N/A
middleware.js gzip 29.9 kB 29.9 kB N/A
edge-runtime..pack.js gzip 844 B 844 B
Overall change 844 B 844 B
Next Runtimes
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
973-experime...dev.js gzip 322 B 322 B
973.runtime.dev.js gzip 314 B 314 B
app-page-exp...dev.js gzip 317 kB 316 kB N/A
app-page-exp..prod.js gzip 125 kB 125 kB
app-page-tur..prod.js gzip 139 kB 139 kB N/A
app-page-tur..prod.js gzip 134 kB 134 kB N/A
app-page.run...dev.js gzip 305 kB 305 kB N/A
app-page.run..prod.js gzip 121 kB 121 kB N/A
app-route-ex...dev.js gzip 31.2 kB 31.2 kB N/A
app-route-ex..prod.js gzip 21.1 kB 21.1 kB N/A
app-route-tu..prod.js gzip 21.1 kB 21.1 kB N/A
app-route-tu..prod.js gzip 20.9 kB 20.9 kB N/A
app-route.ru...dev.js gzip 32.9 kB 32.9 kB N/A
app-route.ru..prod.js gzip 20.9 kB 20.9 kB N/A
pages-api-tu..prod.js gzip 9.62 kB 9.62 kB
pages-api.ru...dev.js gzip 11.5 kB 11.5 kB
pages-api.ru..prod.js gzip 9.61 kB 9.61 kB
pages-turbo...prod.js gzip 20.8 kB 20.8 kB
pages.runtim...dev.js gzip 26.4 kB 26.4 kB
pages.runtim..prod.js gzip 20.8 kB 20.8 kB
server.runti..prod.js gzip 57.7 kB 57.6 kB N/A
Overall change 225 kB 225 kB
build cache
vercel/next.js canary vercel/next.js arlyon/attempt-fix-too-long-file Change
0.pack gzip 1.61 MB 1.61 MB N/A
index.pack gzip 132 kB 132 kB N/A
Overall change 0 B 0 B
Diff details
Diff for page.js

Diff too large to display

Diff for middleware.js

Diff too large to display

Diff for edge-ssr.js

Diff too large to display

Diff for image-HASH.js
@@ -1,7 +1,7 @@
 (self["webpackChunk_N_E"] = self["webpackChunk_N_E"] || []).push([
   [8358],
   {
-    /***/ 8908: /***/ (
+    /***/ 8176: /***/ (
       __unused_webpack_module,
       __unused_webpack_exports,
       __webpack_require__
@@ -9,7 +9,7 @@
       (window.__NEXT_P = window.__NEXT_P || []).push([
         "/image",
         function () {
-          return __webpack_require__(2111);
+          return __webpack_require__(934);
         },
       ]);
       if (false) {
@@ -18,7 +18,7 @@
       /***/
     },
 
-    /***/ 6308: /***/ (module, exports, __webpack_require__) => {
+    /***/ 5057: /***/ (module, exports, __webpack_require__) => {
       "use strict";
       /* __next_internal_client_entry_do_not_use__  cjs */
       Object.defineProperty(exports, "__esModule", {
@@ -30,8 +30,8 @@
           return Image;
         },
       });
-      const _interop_require_default = __webpack_require__(9608);
-      const _interop_require_wildcard = __webpack_require__(4856);
+      const _interop_require_default = __webpack_require__(4345);
+      const _interop_require_wildcard = __webpack_require__(2874);
       const _jsxruntime = __webpack_require__(5815);
       const _react = /*#__PURE__*/ _interop_require_wildcard._(
         __webpack_require__(6180)
@@ -40,17 +40,17 @@
         __webpack_require__(1955)
       );
       const _head = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(2383)
+        __webpack_require__(7163)
       );
-      const _getimgprops = __webpack_require__(5640);
-      const _imageconfig = __webpack_require__(2758);
-      const _imageconfigcontextsharedruntime = __webpack_require__(899);
-      const _warnonce = __webpack_require__(3878);
-      const _routercontextsharedruntime = __webpack_require__(869);
+      const _getimgprops = __webpack_require__(5447);
+      const _imageconfig = __webpack_require__(1650);
+      const _imageconfigcontextsharedruntime = __webpack_require__(1953);
+      const _warnonce = __webpack_require__(5054);
+      const _routercontextsharedruntime = __webpack_require__(4445);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6501)
+        __webpack_require__(7406)
       );
-      const _usemergedref = __webpack_require__(3994);
+      const _usemergedref = __webpack_require__(3303);
       // This is replaced by webpack define plugin
       const configEnv = {
         deviceSizes: [640, 750, 828, 1080, 1200, 1920, 2048, 3840],
@@ -371,7 +371,7 @@
       /***/
     },
 
-    /***/ 3994: /***/ (module, exports, __webpack_require__) => {
+    /***/ 3303: /***/ (module, exports, __webpack_require__) => {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -440,7 +440,7 @@
       /***/
     },
 
-    /***/ 5640: /***/ (
+    /***/ 5447: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -456,9 +456,9 @@
           return getImgProps;
         },
       });
-      const _warnonce = __webpack_require__(3878);
-      const _imageblursvg = __webpack_require__(4317);
-      const _imageconfig = __webpack_require__(2758);
+      const _warnonce = __webpack_require__(5054);
+      const _imageblursvg = __webpack_require__(1731);
+      const _imageconfig = __webpack_require__(1650);
       const VALID_LOADING_VALUES =
         /* unused pure expression or super */ null && [
           "lazy",
@@ -830,7 +830,7 @@
       /***/
     },
 
-    /***/ 4317: /***/ (__unused_webpack_module, exports) => {
+    /***/ 1731: /***/ (__unused_webpack_module, exports) => {
       "use strict";
       /**
        * A shared function, used on both client and server, to generate a SVG blur placeholder.
@@ -885,7 +885,7 @@
       /***/
     },
 
-    /***/ 8580: /***/ (
+    /***/ 9833: /***/ (
       __unused_webpack_module,
       exports,
       __webpack_require__
@@ -911,11 +911,11 @@
           return getImageProps;
         },
       });
-      const _interop_require_default = __webpack_require__(9608);
-      const _getimgprops = __webpack_require__(5640);
-      const _imagecomponent = __webpack_require__(6308);
+      const _interop_require_default = __webpack_require__(4345);
+      const _getimgprops = __webpack_require__(5447);
+      const _imagecomponent = __webpack_require__(5057);
       const _imageloader = /*#__PURE__*/ _interop_require_default._(
-        __webpack_require__(6501)
+        __webpack_require__(7406)
       );
       function getImageProps(imgProps) {
         const { props } = (0, _getimgprops.getImgProps)(imgProps, {
@@ -947,7 +947,7 @@
       /***/
     },
 
-    /***/ 6501: /***/ (__unused_webpack_module, exports) => {
+    /***/ 7406: /***/ (__unused_webpack_module, exports) => {
       "use strict";
 
       Object.defineProperty(exports, "__esModule", {
@@ -982,7 +982,7 @@
       /***/
     },
 
-    /***/ 2111: /***/ (
+    /***/ 934: /***/ (
       __unused_webpack_module,
       __webpack_exports__,
       __webpack_require__
@@ -999,8 +999,8 @@
 
       // EXTERNAL MODULE: ./node_modules/.pnpm/react@19.0.0-rc-7771d3a7-20240827/node_modules/react/jsx-runtime.js
       var jsx_runtime = __webpack_require__(5815);
-      // EXTERNAL MODULE: ./node_modules/.pnpm/next@file+..+main-repo+packages+next+next-packed.tgz_react-dom@19.0.0-rc-7771d3a7-20240827_re_h4bgdqagqtli4q7bnw2ajnn4vm/node_modules/next/image.js
-      var next_image = __webpack_require__(1878);
+      // EXTERNAL MODULE: ./node_modules/.pnpm/next@file+..+diff-repo+packages+next+next-packed.tgz_react-dom@19.0.0-rc-7771d3a7-20240827_re_qz7eh3ruj3cscolfxbcb265hza/node_modules/next/image.js
+      var next_image = __webpack_require__(7649);
       var image_default = /*#__PURE__*/ __webpack_require__.n(next_image); // CONCATENATED MODULE: ./pages/nextjs.png
       /* harmony default export */ const nextjs = {
         src: "/_next/static/media/nextjs.cae0b805.png",
@@ -1030,12 +1030,12 @@
       /***/
     },
 
-    /***/ 1878: /***/ (
+    /***/ 7649: /***/ (
       module,
       __unused_webpack_exports,
       __webpack_require__
     ) => {
-      module.exports = __webpack_require__(8580);
+      module.exports = __webpack_require__(9833);
 
       /***/
     },
@@ -1045,7 +1045,7 @@
     /******/ var __webpack_exec__ = (moduleId) =>
       __webpack_require__((__webpack_require__.s = moduleId));
     /******/ __webpack_require__.O(0, [2888, 9774, 179], () =>
-      __webpack_exec__(8908)
+      __webpack_exec__(8176)
     );
     /******/ var __webpack_exports__ = __webpack_require__.O();
     /******/ _N_E = __webpack_exports__;
Diff for 5867-HASH.js

Diff too large to display

Diff for main-HASH.js

Diff too large to display

Diff for app-page-exp..ntime.dev.js
failed to diff
Diff for app-page-exp..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page-tur..time.prod.js

Diff too large to display

Diff for app-page.runtime.dev.js

Diff too large to display

Diff for app-page.runtime.prod.js

Diff too large to display

Diff for app-route-ex..ntime.dev.js

Diff too large to display

Diff for app-route-ex..time.prod.js

Diff too large to display

Diff for app-route-tu..time.prod.js

Diff too large to display

Diff for app-route-tu..time.prod.js

Diff too large to display

Diff for app-route.runtime.dev.js

Diff too large to display

Diff for app-route.ru..time.prod.js

Diff too large to display

Diff for server.runtime.prod.js

Diff too large to display

Commit: daa1a9b

@arlyon arlyon force-pushed the arlyon/attempt-fix-too-long-file branch 2 times, most recently from 12a0fed to f6caaa2 Compare September 2, 2024 11:43
@arlyon arlyon force-pushed the arlyon/attempt-fix-too-long-file branch from f6caaa2 to 9b81a61 Compare September 2, 2024 11:52
@arlyon arlyon requested a review from bgw September 4, 2024 09:42
@@ -72,6 +72,80 @@ use crate::{
rope::{Rope, RopeReader},
};

pub const MAX_FILE_NAME_LENGTH_UNIX: usize = 255;
Copy link
Member

Choose a reason for hiding this comment

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

Is this going to generate compiler warnings on windows about dead code?

If so, I'd just inline the constant inside of validate_path_length_inner alongside MAX_PATH_LENGTH

Comment on lines 574 to 579
let full_path = validate_path_length(&full_path).with_context(|| {
format!(
"path length for file {} exceeds max length of filesystem",
full_path.to_string_lossy()
)
})?;
Copy link
Member

Choose a reason for hiding this comment

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

Personally, I'd move this .with_context to line 146 inside of validate_path_length where you call validate_path_length_inner.

@bgw bgw merged commit fa53822 into canary Sep 10, 2024
109 checks passed
@bgw bgw deleted the arlyon/attempt-fix-too-long-file branch September 10, 2024 17:02
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Sep 25, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
created-by: Turbopack team PRs by the Turbopack team. locked
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants