Skip to content
This repository

Assets are still compiled when config.assets.compile set false (including in production) #2886

Closed
wants to merge 6 commits into from

7 participants

Mark J. Titorenko Santiago Pastorino Greg Reinacker Guillermo Iguaran Luis Lavena José Valim Prem Sichanugrist
Mark J. Titorenko

During discussions on #RubyOnRails, it was discovered that when config.assets.compile is set to false, assets are still compiled under the following conditions:

  1. config.serve_static_assets is set to false (the default in production) and any request is made for an asset
  2. any request is made for an asset without digest (irrespective of the value of config.serve_static_assets)

This pull request contains a patch to only mount the asset server if config.assets.compile is set to true. In the instance that it is set to false, and a request is made as outlined above, the user agent will receive a 404.

A test for the patch, "assets routes are not drawn when compilation is disabled", is included, as is a small change to the existing test "assets do not require any assets group gem when manifest file is present" which now serves the precompiled asset statically rather than via the assets server.

(Additional credit due to cachemoney on #RubyOnRails for working on this with me.)

Santiago Pastorino
Owner

@mjtko ya that's a good idea but two things ...

1) we can wrap all the after_initialize
2) we need the assets:precompile task to copy each file twice, with and without the digest.
We need rails-#{digest}.png and rails.png because of emails.

Greg Reinacker

I like @spastorino idea 2 - copy each file twice, with and without the digest; I was thinking about this this weekend. It's not just emails - I use my application css file in my 404.html and 500.html files, for example. I need to reference the css path without the digest (since I don't know the digest), and I don't want to use a server-processed css path for something like a 500.html page...

Mark J. Titorenko

@spastorino

1) we can wrap all the after_initialize

Sure, though I figured other libraries, engines and the like may want to make use of the other settings performed in that block - I admit this is somewhat a corner case, but I didn't want to submit a pull request for a change that was considered too sweeping! I can wrap the whole lot though, np.

2) we need the assets:precompile task to copy each file twice, with and without the digest.

Right, I understand why the assets server had been left mounted now. :) In which case, I'll take a look at updating the precompile task too.

Mark J. Titorenko

@spastorino

I took a slightly more complicated approach with the precompile task - I (personally) don't like the idea of cluttering my precompiled assets output with a bunch of duplicate data so, while this is the default, there are 3 other options - do nothing (:none), symlink (err, :symlink) and hard link (:hardlink).

I think this will give people the flexibility to decide what to do but still retains the principle of least surprise.

RFC :)

Guillermo Iguaran

@spastorino Agree with 1)
About 2) I think can be handled separately in another pull request since we must think if there is a better solution for emails. (I don't like to idea of have all the assets duplicated)

Santiago Pastorino

Just wrap the whole after_initialize block with if config.assets.compile

I thought that, but config in that context is the Railtie config object, not the application config object, yielding:

undefined method `assets' for #<Rails::Railtie::Configuration:0x007fe612adc028> (NoMethodError)

:-)

ya you're right :P

Mark J. Titorenko

@guilleiguaran

I agree, I don't like having all the assets duplicated - which is why I took the approach of giving the developer the option of duplicating, not duplicating or linking the 'undigested' assets.

We can shunt it to a separate pull request if that's preferred ofc.

Santiago Pastorino
Owner

I'd remove the configs seems excessive and just copy files because of windows

Santiago Pastorino
Owner

/cc @luislavena is it ok to symlink rails-MD5.png as rails.png on Windows or we should just copy them?

Mark J. Titorenko

@spastorino

Yeah, I defaulted to :duplicate precisely because of the lack of reliable link facilities on MS filesystems. I wanted to provide a configuration option because I can see some people objecting to the idea of duplicating all the assets (eg. @guilleiguaran above!).

If this seems too complicated for now, I can back out those changes and revert the task to a 'dumb copy' approach and we can discuss different approaches in the scope of a different pull request. :)

Luis Lavena

@spastorino while there are hardlinks and symlinks on Windows, Ruby doesn't know about them.

symlink and link will raise NotImplementedError on Windows, so default to :duplicate seems fine with me (there will be no broken experience)

If user has the option to customize, and knows what he is doing with symlinks or hardlinks, then fine.

Santiago Pastorino
Owner

@mjtko please remove the config option and provide another pull request for that

Mark J. Titorenko

Ok, will do. Getting late (early?) here (just after 2am) so will pick this up tomorrow.

Thanks for the comments and suggestions. :)

José Valim
Owner

I believe we need to refactor assets:precompile before adding more code. The whole precompilation code could be moved to a class inside actionpack/lib/sprockets and properly tested without a need to boot a new app everytime.

Guillermo Iguaran

@mjtko your original patch was committed: f22407d, we don't pick the refactor of the second one since it break some tests.

Santiago Pastorino
Owner

This was already merged

Santiago Pastorino spastorino closed this September 13, 2011
Mark J. Titorenko

Ok, thanks. Sorry I didn't pick this up again. If I find some time I'll submit an additional pull request regarding the duplication facility.

Santiago Pastorino
Owner

@mjtko thanks we opt for just leaving the users put their stuff in the public dir so there's not going to be a duplication thing.
Thanks for your help anyway :).

Mark J. Titorenko

Ok, that sounds like a great and simple plan. Thanks for keeping me in the loop. :)

Greg Reinacker

@spastorino unless I'm misunderstanding, it seems like this solution (moving stuff to public) leaves a deficiency for a common use case, when using the asset pipeline defaults (no fallback, digest on).

If one wants to reference a compiled asset (e.g. application.css) from a static HTML file (like an email, or like a 500.html page), then there isn't a way to reference that asset without the digest...and the digest isn't known at authoring time.

Thoughts?

Guillermo Iguaran

@greinacker: are you talking about: #2917?

Actually @spastorino is working on it

Greg Reinacker

@guilleiguaran it's close - ideally I'd want to just be able to point to /assets/application.css from static files and be able to ensure that it will be resolved.

The commit referenced in that issue had a comment with an interesting idea, adding middleware which would lookup your asset request in the manifest and redirect to the digest version...

Santiago Pastorino
Owner

@greinacker definitely the best solution is to make a middleware yeah

Greg Reinacker

@spastorino ya know, thinking about this some more...one of the benefits of precompiling assets is to generate static files that the web server can serve, without involving rails or any middleware. So the middleware solution doesn't really support that. Seems like what we need is a way to say that certain assets should be copied (probably duplicated) without a digest.

This could be taken care of in deployment- right after assets:precompile could be a task to copy some assets...but that just kind of feels kludgy. Seems like the assets:precompile could do the right thing, if we had a way to specify what that right thing was...

Mark J. Titorenko

@greinacker You make a good point about not wanting to involve rails or middleware. I wonder, then, whether a two-pronged attack to solve this issue is required.

It seems to me that the redirect (rather than duplicate) idea has merit as you want to ensure that cache busting still works - another primary use case for the pipeline - so I would suggest:

  1. have a rake task (possibly this could be done in precompile) emit redirection language for your web server of choice (I suppose nginx and apache would be the targets) which can be included within their configuration to provide explicit 302 redirects for digest-less assets. We already have the mapping from digest-less to digested filename in the manifest file, so this would be a translation of that information.
  2. create a middleware as outlined in 82afaa0, which will be called into action as a fallback when accessing the rails app directly.

This has the benefit of keeping the configuration where (I believe) it belongs - in the web server - while providing a sane fallback in the shape of the middleware, in case people don't want to jump through that extra configuration hoop for their web server for whatever reason.

I'm not convinced that this fits in rails core though - perhaps this kind of functionality would be suited to an a subsidiary gem.

How does this sound?

Greg Reinacker

@mjtko adding web server redirect directives feels like we're popping up the stack a little too far. Seems like there are a couple issues with doing that:

  1. My deployment user may not have permission to edit the web server configuration files at deployment time

  2. The web server might need to be restarted to pick up the changes (or at very least have its configuration reloaded), which could cause some downtime (granted, probably only seconds, but it impacts the idea of deploying under load)

  3. If I had hundreds of files I needed to do this with, that could make for a pretty long web server config file. Not sure the impact of that, other than feeling wrong.

It also "feels" complex.

Some alternative ideas:

  1. Change config.assets.digest to take one of :none, :digest, or :both - the first two would correspond to the current false/true options, and the last one would mean assets would be compiled/copied both with and without digests. I realize this copies a lot of files around, but I like this idea the best from the perspective of keeping things simple.

  2. Create symlinks to link from non-digest to digest assets. Somewhat platform-dependent (e.g. problematic on windows).

  3. Have an array to define which assets should additionally be copied without digests; something like config.assets.no_digest += ['logo.png', 'theme/*.png']. Feels kinda kludgy.

I should add that the way 3.1.1 stands at the moment, it's going to break for people using, say, jquery-ui themes, in that their theme-specific images are not going to load with the rails default settings, unless they put the theme into the public directory.

Greg Reinacker

Just want to make sure @spastorino is aware that 3.1.1 will break lots of vendor-ish stuff like jquery-ui themes and such, unless they are put in public (which isn't what the asset pipeline implies you should do) - see end of preceding comment above.

(hope this is ok to put here - not sure how we are supposed to ping about a possible 3.1.1 issue before it's released)

Mark J. Titorenko

@greinacker Sure, I understand where you're coming from with your objections. Let me just take them in order with responses: :)

  1. the web server configuration could be Include'd (in apache speak, i'm sure nginx has a similar capability) from a file within the rails app.
  2. agree here, though if HA is needed, HA should be dealt with somehow else. :)
  3. hundreds of files feels like an edge case where a different approach should certainly be taken. :)

I agree that it's more complex than the approach of copying/duplicating files, though I feel it's more elegant.

To the alternatives!

  1. copying is simple but wasteful IMHO; also breaks cache busting
  2. symlinking also breaks the possibility of cache busting
  3. yup, this feels a bit kludgy indeed

I do like the middleware approach, but I think for 'proper' avoid-the-stack operations (while still retaining cache busting) the only option is to configure the web server to understand the mapping to the digested assets somehow.

Cache busting could be ignored ofc - but isn't this the primary point of using digests in the first place? :-)

Greg Reinacker

@mjtko well...

Re HA: we're not really talking about HA, we're talking about seamless deployments. As one who deploys multiple times throughout the day, the ability to deploy and update throughout the day without users ever really knowing about it, and without having to have load balancer scripts to rotate machines in and out during deployment, is worth a lot. I'd trade a lot of disk space for duplicate files to keep this feature. I guess my thought is this works great now, and breaking this for the sake of elegance might not be worth it.

Re copying files being wasteful - well, you're right. However, if I think about the resources I have to trade off, disk space is one of the cheapest.

You know, I had another idea:

  1. Perhaps each directory in the assets trees could have an optional manifest sort of file, which could list the requirements for the assets in that directory. By default, everything would work like it does now. But if one of more of these manifest files exist, they are processed as well. They could say things like

"this directory's logo.png file should be copied to public/assets without a digest"

or

"everything in this directory should be copied to public/assets without a digest"

Mark J. Titorenko

@greinacker TBH I'm more concerned about the cache busting issues, but, again, I see your point.

Mark J. Titorenko

@guilleiguaran as commented, that's a great if you don't mind hitting the application stack rather than have your web server deal with serving non-digested assets. :)

Greg Reinacker

@guilleiguaran nice work getting that done so quickly! I agree with @mjtko that if you don't mind your application stack handling the request, this is perfect. And, using this gets us to a better state than we were in with 3.1.0, since it uses a redirect rather than falling back to the pipeline and compiling.

Now if only we could all agree on how to do it from the web server. ;-)

Guillermo Iguaran

@greinacker thank to @sikachu, he is the author :)

Prem Sichanugrist
Collaborator

Yeah, as long as you don't mind a request hitting your application server, middleware solution is fine.

For me, I'm deploying to Heroku so middleware is the solution. I don't have another layer of web server such as nginx, apache to look that up for me.

I think the best solution here is actually somehow make the rewrite rule from the manifest file. I don't know if it's possible to reload those rewrite rules on-the-fly or not, but if it is possible that might be the best solution for a apache/nginx user.

Guillermo Iguaran

is a good question, is possible reload rewrite rules on-the-fly?

I personally use this simple Rack app (yes, hitting the framework) in a personal project (probably it has bugs):

class StaticAssets
  def call(env)
    digested_file = Rails.application.config.assets.digests[env['PATH_INFO'][1..-1]]
    filename = File.join(Rails.public_path, Rails.application.config.assets.prefix, digested_file)
    content = File.open(filename)
    [200, {'Cache-Control' => 'no-cache; max-age=0', 'Pragma' => 'no-cache'}, [content.read]]
  rescue
    raise ActionController::RoutingError.new("No route matches #{env['PATH_INFO']}")
  end
end

Rails.application.routes.prepend do
  mount StaticAssets.new => Rails.application.config.assets.prefix
end
Mark J. Titorenko

Researching a bit I can see that it's definitely possible for rewrite rules to be loaded on the fly by Apache. Not yet certain about nginx.

Greg Reinacker

@sikachu just wanted to say nice job on the redirector - worked with it a bit yesterday with 3.1.1rc1, worked exactly as expected.

Prem Sichanugrist
Collaborator
Mark J. Titorenko

Ok, so to (partially) clarify, with Apache, you can use a RewriteMap directive to allow 'dynamically updatable' RewriteRule rules to be used.

As for nginx, it looks to me that it is possible to reload the configuration without interruption to service, though you can't dynamically update rewrites without interfering with the server process at all... http://wiki.nginx.org/NginxCommandLine#Loading_a_New_Configuration_Using_Signals

Santiago Pastorino
Owner

@mjtko @greinacker are you guys happy with the current rc2 status?

Greg Reinacker

@spastorino I'm good - the way this works in rc2 works for me, and in general rc2 feels pretty solid.

Mark J. Titorenko

@spastorino The sprockets app mounting behaviour seems good. Personally, I'm not too happy about having duplicate files in my assets compilation output but perhaps that's just me. I'll roll my own task without the duplication (I have no need for it!) and then everybody is happy.

Perhaps making the non-digested assets optional would be a good plan, but this can wait for 3.1.2. :)

Guillermo Iguaran

agree, I don't need the duplication, if you want I can add the code for it (with an option or using an ENV var). I can do it in 3.1.2 if you don't want to have another rc before the 3.1.1 release :)

Mark J. Titorenko

Well, aren't we going to end up with another rc for some of the changes since rc2 anyway? :) (eg. 258fe7d which REALLY needs to be in 3.1.1 imho :-))

Of course, I'd love for 3.1.1 to be released sooner rather than later. I don't really consider the duplication of files to be a blocking issue - it's not like it's going to cause breakage, it's just unnecessary in a number of common use cases.

Mark J. Titorenko

Gah, also, just found #3198. Down to you core guys to decide if that's a blocker or not. :-/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
This page is out of date. Refresh to see the latest.
22  actionpack/lib/sprockets/assets.rake
@@ -21,6 +21,16 @@ namespace :assets do
21 21
       target = Pathname.new(File.join(Rails.public_path, config.assets.prefix))
22 22
       manifest = {}
23 23
       manifest_path = config.assets.manifest || target
  24
+      case config.assets.undigested_as
  25
+      when NilClass
  26
+        undigested_as = :duplicate
  27
+      when :none, false
  28
+        undigested_as = false
  29
+      when :duplicate, :symlink, :hardlink
  30
+        undigested_as = config.assets.undigested_as
  31
+      else
  32
+        raise "config.assets.undigested_as was '#{config.assets.undigested_as.inspect}'; must be one of: :duplicate, :symlink, :hardlink, :none, false"
  33
+      end
24 34
 
25 35
       config.assets.precompile.each do |path|
26 36
         env.each_logical_path do |logical_path|
@@ -38,6 +48,18 @@ namespace :assets do
38 48
             mkdir_p filename.dirname
39 49
             asset.write_to(filename)
40 50
             asset.write_to("#{filename}.gz") if filename.to_s =~ /\.(css|js)$/
  51
+
  52
+            if config.assets.digest && undigested_as
  53
+              undigested = target.join(logical_path)
  54
+              case undigested_as
  55
+              when :duplicate
  56
+                asset.write_to(undigested)
  57
+              when :symlink
  58
+                symlink(filename, undigested, :force => true)
  59
+              when :hardlink
  60
+                link(filename, undigested, :force => true)
  61
+              end
  62
+            end
41 63
           end
42 64
         end
43 65
       end
2  actionpack/lib/sprockets/railtie.rb
@@ -50,7 +50,7 @@ class Railtie < ::Rails::Railtie
50 50
     # are compiled, and so that other Railties have an opportunity to
51 51
     # register compressors.
52 52
     config.after_initialize do |app|
53  
-      next unless app.assets
  53
+      next unless app.assets && app.config.assets.compile
54 54
       config = app.config
55 55
 
56 56
       config.assets.paths.each { |path| app.assets.append_path(path) }
1  railties/lib/rails/application/configuration.rb
@@ -47,6 +47,7 @@ def initialize(*)
47 47
         @assets.cache_store     = [ :file_store, "#{root}/tmp/cache/assets/" ]
48 48
         @assets.js_compressor   = nil
49 49
         @assets.css_compressor  = nil
  50
+        @assets.undigested_as   = :duplicate
50 51
       end
51 52
 
52 53
       def compiled_asset_path
4  railties/lib/rails/generators/rails/app/templates/config/environments/production.rb.tt
@@ -20,6 +20,10 @@
20 20
   # Generate digests for assets URLs
21 21
   config.assets.digest = true
22 22
 
  23
+  # Method to use to additionally create assets without digests
  24
+  # Valid values are :duplicate, :none, :symlink, :hardlink
  25
+  # config.assets.undigested_as = :duplicate
  26
+
23 27
   # Defaults to Rails.root.join("public/assets")
24 28
   # config.assets.manifest = YOUR_PATH
25 29
 
12  railties/test/application/assets_test.rb
@@ -137,6 +137,7 @@ def app
137 137
 
138 138
     test "assets do not require any assets group gem when manifest file is present" do
139 139
       app_file "app/assets/javascripts/application.js", "alert();"
  140
+      app_file "config/initializers/serve_static_assets.rb", "Rails.application.config.serve_static_assets = true"
140 141
 
141 142
       ENV["RAILS_ENV"] = "production"
142 143
       capture(:stdout) do
@@ -243,6 +244,17 @@ class ::PostsController < ActionController::Base ; end
243 244
       assert_equal 0, files.length, "Expected no assets, but found #{files.join(', ')}"
244 245
     end
245 246
 
  247
+    test "assets routes are not drawn when compilation is disabled" do
  248
+      app_file "app/assets/javascripts/demo.js.erb", "<%= :alert %>();"
  249
+      app_file "config/initializers/compile.rb", "Rails.application.config.assets.compile = false"
  250
+
  251
+      ENV["RAILS_ENV"] = "production"
  252
+      require "#{app_path}/config/environment"
  253
+
  254
+      get "/assets/demo.js"
  255
+      assert_equal 404, last_response.status
  256
+    end
  257
+
246 258
     test "does not stream session cookies back" do
247 259
       app_file "app/assets/javascripts/demo.js.erb", "<%= :alert %>();"
248 260
 
Commit_comment_tip

Tip: You can add notes to lines in a file. Hover to the left of a line to make a note

Something went wrong with that request. Please try again.