"Argument out of range" error on rake assetpack:build

[d-tw]

These changes fix the "argument out of range" error that occurs when attempting to precompile the assets using the assetpack:build rake task.

The "Last-Modified" header value was being returned as a UNIX timestamp which was breaking the Time.parse call used when saving the files.

[j15e]

Which version of sinatra are you using? Can you provide a failing exemple? I can't reproduce the error with a test nor with any rake assetpack:build task I have.

Not sure about this fix as both methods returns a Time object :

File.mtime(file_name) → time Returns the modification time for the named file as a Time object.

Time.at(time) → time Creates a new time object with the value given by time, the given number of seconds_with_frac, or seconds and microseconds_with_frac from the Epoch.

And last_modified helper do deal with time objects :

Set the last modified time of the resource (HTTP ‘Last-Modified’ header) and halt if conditional GET matches. The time argument is a Time, DateTime, or other object that responds to to_time.

http://sinatra.rubyforge.org/api/classes/Sinatra/ResponseHelpers.html#M000036

[j15e]

Referenced wrong issue, re-opening.

[d-tw]

The issue occurs in the following environment:

• ruby 1.9.3-p374
• rack 1.4.4
• sinatra 1.3.3

Whilst both methods return a Time object, the current code immediately converts them to Fixnums:

At lib/sinatra/assetpack/class_methods.rb:28, package.mtimereturns a Fixnum, as it is the result of lib/sinatra/assetpack/buster_helpers.rb:15 using the to_i method. I contemplated removing the to_i call, but the result is used to generate the cache buster tag (the whole purpose of the method), so I opted to convert it back to a Time object later on.

At lib/sinatra/assetpack/class_methods.rb:52, the Time object is converted to a Fixnum via to_i. My change could have been improved (I wasn't paying attention with that one). Instead of:

last_modified Time.at(File.mtime(fn).to_i)

I should have used:

last_modified File.mtime(fn)

So, in both cases, the current code doesn't actually pass Time or DateTime, instead it's passing a Fixnum.

[j15e]

Thanks, that is what I thought for lib/sinatra/assetpack/class_methods.rb:52.

For the package, we should change lib/sinatra/assetpack/class_methods.rb:28 as we will be moving to MD5 very soon and mtime won't be used for this anymore.

Still, I can't reproduce your issue with your specific versions (or my test isn't testing your issue), see 70464df04c2c4a7dc69bbfe5eb7dfb3fd095d2d3

(testing with 1.9.3p327, I'll try 1.9.3p374 asap)

[d-tw]

I have reverted the original changes, and created a new commit with the improved changes, as discussed.

[j15e]

Great, may you rebase it on master & squash all your commits into 1 so I can make a clean merge?

I am still a bit reluctant to merge this without a failing test, do you have a sample application with this bug? Do you think it is specific to 1.9.3-p374? I am still compiling it atm

[j15e]

(and just push force it to avoid duplicate PR)

[d-tw]

I agree. It would be better if there were a reproducible test case. I'll have a look tomorrow to see if I can build one and write some tests against it.

I think this should be attempted before pulling the commits in.

[j15e]

I tried with ruby 1.9.3p374 but not issue on my side either

[d-tw]

Git isn't cooperating with the rebase. Probably because it's already been pushed. I'll close this Pull Request, and initiate a new one once I've managed to get a working test case.