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

Windows builds broken: Rollup creates files with colons in the name #16

Open
4 of 7 tasks
NfNitLoop opened this issue Feb 21, 2021 · 13 comments
Open
4 of 7 tasks
Assignees
Labels
bug Something isn't working next-release This feature will be part of the next release upstream Waiting on a bug fix in an upstream dependency

Comments

@NfNitLoop
Copy link
Owner

NfNitLoop commented Feb 21, 2021

Summary

When Snowpack generates files via snowpack build, it ends up generating files that have colons in the name.

Initially, these seem to work when I'm running the rust server in dev. mode, which passes through file requests to the underlying file system. However, when I attempt to do a release build, which bundles files up into the executable, the required files do not exist.

This also breaks the ability for rollup to do a release build. It itself can't find the files it generated. 😣

Steps to reproduce

Take a look in build/web_modules/common

  • On Linux/MacOS, you'll likely see a file named __node-resolve:empty-1777c4cc.js
  • ⚠️ But on Windows the file will be called __node-resolve. It will be 0 bytes.

This is because file names can't have a colon in them on Windows. However, file paths can have a colon in them, which causes the bit after the colon to be interpreted as a stream when using NTFS file systems. You can see this in this output:

C:\Users\Cody\code\feoblog\web-client\build\web_modules\common>dir /r __node-resolve
 Volume in drive C has no label.
 Volume Serial Number is 3894-D9B7

 Directory of C:\Users\Cody\code\feoblog\web-client\build\web_modules\common

02/20/2021  09:41 PM                 0 __node-resolve
                                   358 __node-resolve:empty-1777c4cc.js:$DATA
               1 File(s)              0 bytes
               0 Dir(s)  357,425,332,224 bytes free

This issue still exists even if I upgrade to Snowpack v3.

I'm not sure if this is a Snowpack bug or a Rollup bug, so I've just reported it here against my own code until I find the culprit. :)

A bit more detail

This seems to work in dev mode because the expected file name is directly requested via the browser. RustEmbed then asks the filesystem for that file path, which also ends up opening the NTFS data stream.

However, bundling the files likely just lists files on disk and tries to include them in the binary. Likewise, copying or zipping the files would probably break this functionality. Right now this is a blocker to having a working feoblog web client on Windows. 😢

See:

Current Status

To do:

  • Work with the Rollup team to merge and release Don't allow : in file names. rollup/rollup#3972
  • Ping the Snowpack team to create a release w/ that version.
  • Update Feoblog to use that version of Snowpack
  • Abandon Snowpack, switch to esbuild.
  • Confirm it works on Windows.
  • Enable Windows build automation.
  • Release a Windows build
@NfNitLoop
Copy link
Owner Author

I see this seemingly-related line over in the rollup node-resolve plugin:
https://github.com/rollup/plugins/blame/master/packages/node-resolve/src/index.js#L15

const ES6_BROWSER_EMPTY = '\0node-resolve:empty.js';

I'm not quite sure what's going on there, but I think from looking at other code in that repo that \0 is specifically used because it's not a valid filename in any file system. Sooo... I'm not sure how a similarly-named file is ending up on my filesystem.

@thgh -- you were the last person to touch that line. Any ideas? 😄 🤞

@NfNitLoop NfNitLoop mentioned this issue Feb 21, 2021
3 tasks
@NfNitLoop
Copy link
Owner Author

NfNitLoop commented Feb 21, 2021

Aha, interesting. Over in rollup/rollup#3342 some tests were recently updated to no longer use : in that \0-prefixed string.

https://github.com/rollup/rollup/pull/3342/files#diff-478c0f90009219e922bc7f42abeb14c8b74fea727890acb28787d768caf1edbdL6

- var _virtual_entry1 = exports('default', "\u0000virtual:entry-1");
+ var _virtualEntry1 = exports('default', "\u0000virtual-entry-1");

At the moment, Windows tests are no longer running because Github actions do not want to check out files names containing ":". I changed the corresponding test [...]

Is this a case of the test being updated, but the convention not being updated in the many places that use it?

@NfNitLoop
Copy link
Owner Author

I've been doing a bit of investigation on my snowpack-v3 branch, which introduces another invalid file name:

C:\Users\Cody\code\feoblog\web-client\build\_snowpack\pkg\common>dir /r polyfill-node
 Volume in drive C has no label.
 Volume Serial Number is 3894-D9B7

 Directory of C:\Users\Cody\code\feoblog\web-client\build\_snowpack\pkg\common

02/22/2021  11:32 AM                 0 polyfill-node
                                54,684 polyfill-node:buffer-ec14102c.js:$DATA
               1 File(s)              0 bytes
               0 Dir(s)  357,233,676,288 bytes free

This seems possibly related to this line of code?

https://github.com/snowpackjs/rollup-plugin-polyfill-node/blob/main/src/index.ts#L8

@NfNitLoop
Copy link
Owner Author

OK, I think I've got it. rollup has this:

export function sanitizeFileName(name: string): string {
	return name.replace(/[\0?*]/g, '_');
}

which strips the \0 characters, but leaves in the : character. It should probably do the same for :. I'm going to tryyyy to write a test for this, but I've spent all day trying to just get npm link to work so I could add debugs statements to my rollup invocations and failed soo... wish me luck. (Or let me bribe you for some help!)

@NfNitLoop
Copy link
Owner Author

And it's not that simple, because while the function is called sanitizeFileName it's actually being used to sanitize full paths, which may contain a drive letter prefix like C:. I've modified an existing test and am running tests locally now. Lots of tests fail on my machine before I even change anything. (ex: the one that tests symlinks, which don't work on NTFS.) I'm curious how those are working in the windows Github Actions runner. 🤔

@thgh
Copy link

thgh commented Feb 24, 2021

Several Rollup plugins use "virtual modules" (starting with \0) as recommended in the Rollup documentation. These virtual modules must be resolved & loaded in the plugin itself, otherwise they will end up as "external". No idea what's going on here though.

@NfNitLoop NfNitLoop added bug Something isn't working upstream Waiting on a bug fix in an upstream dependency labels Feb 25, 2021
@NfNitLoop NfNitLoop changed the title Snopwpack/Rollup/something creates files with colons in the name Windows builds broken: Rollup creates files with colons in the name Feb 25, 2021
@guybedford

This comment has been minimized.

@NfNitLoop
Copy link
Owner Author

Wooo, looks like this might be fixed soon. 🤞

FredKSchott/snowpack#3606

@NfNitLoop
Copy link
Owner Author

So that fix got released in 3.8.5. I'm trying out 3.8.8 but ran into this blocker:
FredKSchott/snowpack#3712

@NfNitLoop
Copy link
Owner Author

Wooo, looks like this might be fixed soon. 🤞

Nope. Have to wait for new releases of esinstall and snowpack. See: FredKSchott/snowpack#3606 (comment)

@NfNitLoop
Copy link
Owner Author

Opened a new bug for Snowpack to request they actually bump the dependency version so we can use the fix. :p

@NfNitLoop
Copy link
Owner Author

Well, we're coming up on a year of this issue being open. I think I'm going to try some Snowpack alternatives. Vite looks like a good one to try first: https://vitejs.dev/

@NfNitLoop
Copy link
Owner Author

Had some issues with Vite. And Rollup. And webpack.

But ESBuild (with some plugins) was easy to get working, and builds a fully bundled and minified app fast enough that I can just use that for dev mode.

So the next version of FeoBlog should be buildable and available on Windows. :D 🎉

@NfNitLoop NfNitLoop self-assigned this Mar 7, 2022
@NfNitLoop NfNitLoop added the next-release This feature will be part of the next release label Mar 19, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working next-release This feature will be part of the next release upstream Waiting on a bug fix in an upstream dependency
Projects
None yet
Development

No branches or pull requests

3 participants