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

[mjtko]

Patch and associated test to fix #3198.

[josevalim]

/cc @spastorino

[spastorino]

@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]

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]

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]

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

[josevalim]

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]

@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]

Ok, I am working on fixes to both problems.

[josevalim]

All fixed.

[spastorino]

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

[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. ;)