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

Sprockets 3.3.2+ no longer works on Windows #115

Closed
daniel-rikowski opened this issue Aug 25, 2015 · 21 comments
Closed

Sprockets 3.3.2+ no longer works on Windows #115

daniel-rikowski opened this issue Aug 25, 2015 · 21 comments

Comments

@daniel-rikowski
Copy link
Contributor

Sprockets 3.3.2+ doesn't work on Windows anymore, because it tries to load non-existent asset files.

I tried to track down this problem, but no success. Here's what I found so far:

It looks like sometimes asset_from_cache for some reason retrieves an asset from cache with absolute load_path and filename (AFAIK this is not intended, this might be related to 436e2d6)

The following call to expand_from_root additionally "expands" these files to paths like C:/Projects/Rails/C:/Projects/Rails/app/assets/javascripts/application.js which subsequentially cause exceptions in File.binread.

Clearing the cache does not fix the problem.

My guess is that somewhere a Windows path is not recognized as absolute and instead treated like a relative path. (because of the missing leading slash)

PS: This was reported at sstephenson#736, too

@schneems
Copy link
Member

Downgrade to 3.2 while I work on a fix please

@daniel-rikowski
Copy link
Contributor Author

Thank you!

I downgraded to 3.3.1 a few days ago. Despite intermittent share violations regarding cache files, that release works for me.

schneems added a commit that referenced this issue Aug 25, 2015
Windows uses a filesystem that starts with a drive letter than a colon and a slash. This is different from *nix systems that only start with a slash. Previous releases 3.3.{1,2,3} didn't take this into consideration. 

To support windows we have to make sure we can correctly expand both kinds of paths. Previously we were checking if a path started with a slash to indicate it was already an absolute path and that we don't need to prepend the root to it. Now we also check for a drive letter + colon + slash. I added tests for windows style paths and this works for compressing them. The downside is that someone could create a folder in their app root called `C:` and this code would incorrectly think that the path was absolute and not expand it. I would assume this would happen in an EXTREME minority of cases, but when it does happen it will be very frustrating to find and fix. We could explicitly check for when you're on a windows machine using `Gem.win_platform?` but I don't know how accurate this code is, and it would make running tests on *nix platforms difficult.

In addition to expanding uris, we also have to support compressing them. The root object we get from windows will not contain a beginning slash, it will be something like `C:/Path/to/root`. Unfortunately the path code we've written will produce paths with beginning slashes. If the path starts with a slash we must make sure the root starts with a slash. If we don't  then when we try to get a compressed path from `file:///C:/Projects/Rails/app/assets/javascripts/application.js` which has a path of `/C:/Projects/Rails/app/assets/javascripts/application.js` it will not correctly compare with it's root which is `C:/Projects/Rails/`.  I'm pretty sure this doesn't impact *nix systems as their root will always begin with a slash.

Added tests for the new behavior.
@schneems
Copy link
Member

I have a patch at #116 if you want to take a look.

Can you try out my patch and let me know if your machine works with it. In your Gemfile add:

gem "sprockets", github: "rails/sprockets", branch: "schneems/fix-windows-3.x"

Let me know if that works. I tried running tests on a windows VM but they don't pass even on 3.2 before this path compression/expansion stuff was introduced.

@daniel-rikowski
Copy link
Contributor Author

The good news:

No more errors with missing files.

The bad news:

The assets are recompiled for every action, i.e. it takes 40 seconds to render each and every view.

I used a debugger to see what's going on, but to no success. The cache files are written and it looks like they are fetched, too, but for some reason load_from_unloaded is still called.

It might be irrelevant, but I noticed that 3.3.1 created about 1,600 files in the cache directory while your version creates about 2,100. Maybe cached assets are stored and fetched with different keys?

@daniel-rikowski
Copy link
Contributor Author

I forked sprockets and tried to run the tests.

A lot of failures are because the tests themselves are not compatible with Windows.

  1. All digest and encoding related tests can be fixed by forcing the line end to LF. By default Git auto-converts them to CRLF on Windows. Adding test/fixtures/**/* eol=lf to .gitattributes fixes that.
  2. Many tests compare URIs using the pattern "file://#{fixture_path("...")}?more=stuff". These tests all fail on Windows because Sprockets returns file:///C:... (triple slash)
  3. After replacing all these URIs and reducing the number of failed tests from 70 to 26 I found a few failing tests which might hint to the cause of the cache problem:

bundle single stylesheet file: This tests expects a single digest, but in fact it gets two:

-["file-digest://C:/Projekte/sprockets/test/fixtures/asset/project.css"]
+["file-digest:///C:/Projekte/sprockets/test/fixtures/asset/project.css", "file-digest://C:/Projekte/sprockets/test/fixtures/asset/project.css"]

asset is fresh if its mtime is changed but its contents is the same In this test the URI gets modified:

-"file:///C:/Projekte/sprockets/test/fixtures/asset/test-POW.png?type=image/png&id=5f9dd91f8d618e5eea6901efc142ebc6083058bf84a0528f0411c231e4d1717b"
+"file://C:/Projekte/sprockets/test/fixtures/asset/test-POW.png?type=image/png&id=5f9dd91f8d618e5eea6901efc142ebc6083058bf84a0528f0411c231e4d1717b"

Perhaps this helps.

schneems added a commit that referenced this issue Aug 26, 2015
@schneems
Copy link
Member

It might be irrelevant, but I noticed that 3.3.1 created about 1,600 files in the cache directory while your version creates about 2,100. Maybe cached assets are stored and fetched with different keys?

That's no good. Sorry about that We want the 3.3 series to behave similarly to 3.2 but allow the project to be copied to different directories and take advantage of the cache. Can you give me an example app that reproduces this problem so I can debug it on my VM?

Re: the test suite. While I can technically do things on a VM it is SOOOOO slow and I lose things like my keyboard shortcuts which kill my productivity. I wasn't planning on fixing the windows suite but having it fixed would make supporting windows much easier in the future. Thanks for your work looking into the tests, if you can implement 1 and 2 and get them to pass on travis (linux) as well I will send a large pizza with whatever you want on it to anywhere you want (seriously) 😄

I think you're right with those last two issues causing the cache problem. I merged my PR into the 3.x branch. I would like to fix this new problem before cutting a new release. Having a project that reproduces the files getting re-compiled would help me a lot to start digging in more.

schneems added a commit that referenced this issue Aug 26, 2015
@schneems schneems reopened this Aug 26, 2015
@schneems
Copy link
Member

I tried to reproduce the crazy files in the cache directory and wasn't able to. I used this app: https://github.com/schneems/absolute_path_sass_issue

I deleted the tmp/cache and public/assets directories and ran bundle exec rake assets:precompile multiple times. I then investigated the folder with finder:

It doesn't matter how many times I run that precompile task, it doesn't add any more cache files.

I also tried clearing the cache and assets directory and running in development mode. I'm still only seeing 28 cache entries.

Try deleting your tmp/cache directory if you're using my branch, it may still have older cache fragments in there from 3.3.3. Normally when you upgrade sprockets the cache gets cleared, but since the VERSION number hasn't changed in my branch, it might not get cleared.

I'm only seeing URIs that start with three slashes come out of load_from_unloaded which I believe is the same behavior of 3.2.

@schneems
Copy link
Member

Any luck reproducing the problem?

@daniel-rikowski
Copy link
Contributor Author

Sorry, I was "AFK" today.

I was partially able to reproduce it with a trivial rails new Demo and rails scaffold Posts. I got a different number of cache files depending on wether I'm using 3.3.1 or schneems/fix-windows-3.x, i.e. 44 or 53. But I just started the application and accessed a view instead of using assets:precompile, so I'll recheck that.

Just to make sure we are talking about the same phenomenon: There was never any continuous growth in the number of files, but a different fixed number depending on the version of Sprockets.

I noticed that after your last commit 573754b the number of additional cache files in my original application has dropped, from about 2,100 to now 1,800, which is a lot closer to the original 3.3.1 number of 1,600 (I assume that some assets were stored as both file://c:/... and file:///c:/...)

I was not able to reproduce the "rebuild on every request" problem yet, I'll work on that tomorrow.

PS: Regarding the test suite: I'll see what I can do 😃

@schneems
Copy link
Member

No worries. I asked my boss for a windows machine so I don't have to use a VM, we'll see where that goes.

There should be nothing in the cache with ///c: unless it's in your gem directory (or otherwise out of the "root" of your app). Everything going in should be // after it's been compressed and everything once it's read from the cache should be ///c:. If not, it's a bug.

Let me know if you can repro the bug, or if you need help, or if you can't work on it anymore. If there's a bug there I would like to get it patched before cutting a new sprockets release.

@daniel-rikowski
Copy link
Contributor Author

I rechecked the number of files using rake assets:precompile and it gets even weirder.

I tested the following scenarios with the simple demo app I previously mentioned:

  1. Run rake assets:precompile
  2. Render a view in the browser
  3. Run rake assets:precompile followed by rendering a view in the browser

I did this with both 3.3.1 and schneems/fix-windows-3.x and also on both CentOS 7 and on Windows 10. (i.e. 12 sets of caches)

The good news: On CentOS the number of files in the cache were always the same, regardless of the scenario and the version of Sprockets: 49 files.

But on Windows it gets weird:

Sprockets 3.3.1:
precompile: 49 (same as CentOS!)
render view: 32
precompile + view: 70

schneems/fix-windows-3.x:
precompile: 49 (same as CentOS!)
render view: 59
precompile + view: 59

Judging from these numbers I'd say that the Github version looks a lot more consistent.

Next I'll work on the test suite, maybe I can track down the reason for this behaviour. Perhaps fixing this also fixes the constant asset rebuilding problem.

@schneems
Copy link
Member

3.3.1 is busted. Testing against it doesn't tell me much. If you want to check against a known working product 3.2.0 would be a good version.

I merged my branch, so you can use the 3.x branch to test instead of schneems/fix-windows-3.x

@daniel-rikowski
Copy link
Contributor Author

Sprockets 3.2.0:
precompile: 49 (same as CentOS)
render view: 34
precompile + view: 49 (same as CentOS)

@daniel-rikowski
Copy link
Contributor Author

tl;dr: I found the problem, but there is no easy fix.

I noticed that the same asset has different urls depending on whether it has been loaded from cache or not, e.g from test_asset.rb:

@env['test-POW.png'].uri # 1st call, unloaded asset, returns: file:///c:/projects/...
@env['test-POW.png'].uri # 2nd+ call, cached asset, returns: file://c:/projects/...

This specific problem is caused by URITar: When the asset is fetched from cache and its file:// uri is expanded again, the necessary leading slash is missing. Unfortunately just slapping it on in URITar#expand doesn't work, because it is also used to expand local file names, i.e. @env['test-POW.png'].filename would return /c:/projects/... afterwards, which doesn't work with local file operations, e.g. File.binread.

To fix this URITar would have to return different values, depending on whether they are used for file:// urls or local file names.

(Incidentally this is the same problem the test suite was suffering on Windows, i.e. #121)

PS: I'm still working on the "green" test suite on Windows, but a lot of tests fail because of the URI compression, so I'll put it on hold for a while.

@schneems
Copy link
Member

Thanks a ton! That makes sense. Looks like my test for URITar wasn't handling that case which is sad because that's 90% of what it does. I'll take a look at a fix.

@schneems
Copy link
Member

I think i've got a fix, can you take a look and try out #123. Hopefully that should be the last of it 🙊

@daniel-rikowski
Copy link
Contributor Author

Well, it fixes a few tests, but breaks a lot of others, notably all resolve related tests: 😭

Example:

resolve('foo.js')

# Before:
"file:///C:/Projekte/sprockets/test/fixtures/resolve/javascripts/foo.js?type=..."

# After:
"C:/Projekte/sprockets/test/fixtures/resolve/javascripts/foo.js"

An empty scheme does not necessarily mean that a local file name is requested.

I fear the only solution is to split expand into expand_uri and expand_pathname (or something).

@schneems
Copy link
Member

I'm not seeing the same thing. If i manually change all the "file://#{fixture_path to "file:///#{fixture_path in test/test_resolve.rb and run

$ bundle exec rake test TEST=test/test_resolve.rb

Then it all passes on my branch.

I'm not totally following your example . That patch shouldn't affect if a scheme is returned by a call to resolve or not. Maybe i'm missing something or did something wrong. Can you give me some more context or some code to try?

BTW, I really appreciate your help in all of this, thanks for all your time and attention.

@daniel-rikowski
Copy link
Contributor Author

I am so sorry, it looks like it had a broken test suite.

If i manually change all the ... Then it all passes on my branch.

This is what I should have done, too. Instead I checked out your branch and then checked out the test suite from my master via git checkout master test. (I still believe this should have worked, but it did not.)

BTW, I really appreciate your help in all of this

Thank you very much! I'm just happy to give something back to the community.

@schneems
Copy link
Member

No worries. Thanks for the sanity check. If it wasn't for you, who knows how much longer windows would continue to be broken 🚀

I just merged my PR in. I'll cut a release soonish with that new code. Try to Let me know if anything else is failing on your test PR that might be a legit failure rather than the test suite not working with windows when you get the chance.

@daniel-rikowski
Copy link
Contributor Author

After the recent 3.4 release I took another look at the test suite and there were no more errors in the test suite, except for the Unix skips (And some SassC load errors, because I can't install the gem on Windows...)

As far as I can tell, none of the other bugs have re-appeared, too (directory growth, constant recompilation, ...) except the one with threaded servers (#136)

I also took the liberty to squash and rebase the test suite pull request (#121)

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

No branches or pull requests

2 participants