rails / sprockets

Rack-based asset packaging system
MIT License
954 stars 790 forks source link

Sprockets 3.3.2+ no longer works on Windows #115

Closed daniel-rikowski closed 9 years ago

daniel-rikowski commented 9 years ago

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 436e2d6c0b9dc543e33612816dd0e69ad88dffae)

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 https://github.com/sstephenson/sprockets/issues/736, too

schneems commented 9 years ago

Downgrade to 3.2 while I work on a fix please

daniel-rikowski commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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) :smile:

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 commented 9 years ago

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 commented 9 years ago

Any luck reproducing the problem?

daniel-rikowski commented 9 years ago

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 https://github.com/rails/sprockets/commit/573754b47484a8b9597ae41a4ce7463d6841ae40 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 :smiley:

schneems commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

daniel-rikowski commented 9 years ago

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. https://github.com/rails/sprockets/pull/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 commented 9 years ago

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 commented 9 years ago

I think i've got a fix, can you take a look and try out https://github.com/rails/sprockets/pull/123. Hopefully that should be the last of it :speak_no_evil:

daniel-rikowski commented 9 years ago

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

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 commented 9 years ago

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 commented 9 years ago

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 commented 9 years ago

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

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 commented 9 years ago

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)