Skip to content

HTTPS clone URL

Subversion checkout URL

You can clone with
or
.
Download ZIP

Loading…

(Fix) Asset pipeline unexpectedly removes digest assets - 3.1.1.rc2 #3199

Closed
wants to merge 3 commits into from

4 participants

@mjtko

Patch and associated test to fix #3198.

@josevalim
Owner
@spastorino
Owner

@mjtko what about --trace?

@mjtko

While I agree that it would be great to support --trace, it didn't work before either - at a guess it's pruned from the ARGV Array somewhere up in the rake code. How much do you want the subtasks to support --trace? Do you know of any way to interrogate the rake environment for it's current --trace setting?

Putting that to one side, I have found a related issue - with the ruby;exit invocation, you lose any subsequent tasks from the command line. eg. rake assets:precompile assets:clean - while a dumb example, it demonstrates the point - I would expect to be left with no assets at the end of that command but, of course, they're all still there.

I think perhaps a less trivial fix is required for this issue.

My feeling is that we should:

  1. only execute assets:precompile (like we are as of this patch)
  2. not be exit-ing directly afterwards (need to look back at the earlier issue where this was incorporated) so as not to break the next tasks - I don't think adding them to the re-invocation is sensible because the environment has been messed with by this point.

Will take a look to see what I can do and update my fork shortly.

While we need to patch up this task for 3.1.1, this is certainly smelling more and more like it should be some kind of rewrite very soon!

@josevalim
Owner

Hrm, maybe --trace was/is not going to work because the output of the ruby is not outputting to the $stdout of the original process?

@mjtko

need to look back at the earlier issue where this was incorporated

Caught between a rock and a hard place - we have to exit or enhancements are run multiple times; we can't exit because then later tasks are either (before this patch) run in the wrong environment or (after this patch) not run at all.

I guess we're left with either:

  • emit a warning that things aren't going to happen as the user expects

or:

  • run an additional rake re-invocation with the remainder of the tasks to run after precompile after restoring the environment

Do you have a preference or another suggestion?

@josevalim
Owner

I would emit a warning. If we try to be smart and detect tasks we are going to get bitten by it eventually.

@mjtko

@josevalim --trace works if I manually add it to the argument list - it's just not present in ARGV fsr. On further investigation it seems that rake uses OptionParser which swallows all the options. Nice(!)

I was briefly concerned about --dry-run, but luckily that's ok. :) I've found a way of determining the --trace setting so will incorporate that and a warning about subsequent tasks into my fork.

I'll open up an additional pull request containing the alternative approach so we can consider it during post 3.1.1 refactoring.

@josevalim
Owner

I also have a proposal that may solve many of the issues we are describing now. Let me prepare a patch as well.

@mjtko mjtko raise and fail fast if assets:precompile is not the last task supplie…
…d on the rake command line; support --trace for re-invocations
cbf4b93
@josevalim
Owner

Here is a gist that should solve #3198 and still handle the exit problem properly:

https://gist.github.com/9b244cdf1c8361be4509

@mjtko

Well, there's my attempt. I'm sure yours will be significantly better. :-)

@mjtko

Here is a gist that should solve #3198 and still handle the exit problem properly:

Ah, nice idea! Still the slight (existing :-)) problem with the environment being chewed up for subsequent tasks, but I guess this is pretty much a corner case and it should be relatively trivial to do a save/restore as appropriate anyway.

@spastorino spastorino closed this
@spastorino
Owner

@mjtko @spohlenz @guilleiguaran can you guys confirm that this 2db49c5 is ok for you guys?

@mjtko

I'm a bit concerned about precompile relying on clean as that's a change in behaviour (reasonable though it sounds). Perhaps bump that for now? Willing to defer to rails-core's better judgement on that though of course! :)

@mjtko

You could choose to pull in the test from 6759a88 too... :)

Edit: LOL - just noticed @josevalim already did that - please ignore me. =]

@spohlenz

@spastorino It's a little worse as now the enhance block runs in the wrong environment (i.e. the initializers have been run before RAILS_GROUPS and RAILS_ENV were set).

I can work around that using an extra rake task and the ruby re-invocation dance though.

@mjtko

I've done a bit more testing this morning and there are still some flaws regarding assets:clean.

  1. (latest) assets:clean does not initialize the environment so assumes an asset prefix of 'assets'
  2. (previous) assets:clean initializes the wrong environment so assumes the asset prefix of development, rather than the asset prefix definied in production

I'm kinda at the end of my tether with this task now. It's going to get refactored.

@spohlenz

@spastorino That commit also breaks the test "precompile properly refers files referenced with asset_path and and run in the provided RAILS_ENV", not that I'm particularly bothered by that change in behaviour.

@josevalim
Owner
@josevalim
Owner
@spastorino
Owner

@mjtko @spohlenz can you guys check again that this 360b8c4 is good for you? :heart: :heart: :heart:

@spohlenz

Thanks @josevalim, @spastorino. I can confirm this solves the issues for me. Enhancing assets:precompile:all works and achieves the functionality I need.

@mjtko
  1. (latest) assets:clean does not initialize the environment so assumes an asset prefix of 'assets'

Unfortunately, this still doesn't work. This is primarily because the clean task is running in the development environment by default while the precompile task is running in the production environment by default.

I'm about to open up a couple of pull requests - one containing a failing test to cover this case and one containing that test plus a bunch of refactoring work to this task and the Sprockets::StaticCompiler. Pull wisely. ;)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Commits on Oct 3, 2011
  1. @mjtko

    add failing test for #3198

    mjtko authored
  2. @mjtko
  3. @mjtko

    raise and fail fast if assets:precompile is not the last task supplie…

    mjtko authored
    …d on the rake command line; support --trace for re-invocations
This page is out of date. Refresh to see the latest.
View
10 actionpack/lib/sprockets/assets.rake
@@ -4,9 +4,15 @@ namespace :assets do
# We need to do this dance because RAILS_GROUPS is used
# too early in the boot process and changing here is already too late.
if ENV["RAILS_GROUPS"].to_s.empty? || ENV["RAILS_ENV"].to_s.empty?
+ unless ARGV.index('assets:precompile') == (ARGV.length - 1)
+ # assets:precompile is not the last task, but we're about to
+ # exit early, so let's fail fast here and show an explanatory
+ # message.
+ raise "The assets:precompile may not be followed by other tasks. Please run multiple rake invocations instead."
+ end
ENV["RAILS_GROUPS"] ||= "assets"
ENV["RAILS_ENV"] ||= "production"
- ruby $0, *ARGV
+ ruby $0, *([Rake.application.options.trace ? '--trace' : nil, 'assets:precompile'].compact)
exit
else
require "fileutils"
@@ -42,7 +48,7 @@ namespace :assets do
YAML.dump(manifest, f)
end
ENV["RAILS_ASSETS_NONDIGEST"] = "true"
- ruby $0, *ARGV
+ ruby $0, *([Rake.application.options.trace ? '--trace' : nil, 'assets:precompile'].compact)
exit
end
end
View
13 railties/test/application/assets_test.rb
@@ -425,6 +425,19 @@ class ::PostsController < ActionController::Base ; end
assert_equal 1, output.scan("enhancement").size
end
+ test "digested assets are not mistakenly removed" do
+ app_file "public/assets/application.js", "alert();"
+ add_to_config "config.assets.compile = true"
+ add_to_config "config.assets.digest = true"
+
+ quietly do
+ Dir.chdir(app_path){ `bundle exec rake assets:clean assets:precompile` }
+ end
+
+ files = Dir["#{app_path}/public/assets/application-*.js"]
+ assert_equal 1, files.length, "Expected digested application.js asset to be generated, but none found"
+ end
+
private
def app_with_assets_in_view
Something went wrong with that request. Please try again.