Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Support JSON manifests and manifests with randomized filenames #412

Merged
merged 5 commits into from Apr 23, 2013

Conversation

Projects
None yet
8 participants
Contributor

jimryan commented Mar 8, 2013

Rails 4 now writes a JSON asset manifest file with a randomized filename. This commit expands the deploy:assets tasks to support it. It is completely compatible with YAML manifests as well as manifests that do not have a randomized filename.

Rollbacks are still broken, though, because there doesn't appear to be an assets:precompile:nondigest task or equivalent in Rails 4. I'll have to look into that further.

Fixes #362

This patch worked for me.

working, thanks!

xanview commented Mar 15, 2013

I tried to add gem 'capistrano', github: "jimryan/capistrano" to my Gemfile but I'm getting the same manifest.yml not found error :( -- Any ideas?

Try it like this:

gem 'capistrano', :git => "git://github.com/jimryan/capistrano.git", :branch => "support-json-manifest"

xanview commented Mar 16, 2013

Thanks, worked beautifully! :) -- Looking forward to seeing this merged!

Emerson commented Mar 21, 2013

Also wanting to see this merged...

hardbap commented Mar 25, 2013

There is still the issue of deploying to multiple servers. The manifest-*.json will be unique to each server and shared_manifest_path uses Cap's capture which will only gets the manifest file name from the first server.

[EDIT: removed tip about fix because I need to test it.]

Contributor

jimryan commented Mar 25, 2013

That's a good point. In a multi-server scenario, you could remove the random portion of the filename as you originally suggested (you were right), or you could manually sync the manifest filenames (Sprockets doesn't care what the random part is, and it won't change it once it's there). I suppose that cap could/should aid in the latter in some way.

Contributor

jimryan commented Mar 26, 2013

That's definitely a solution that's probably not an issue right now, but I'd read through this thread here on the commit that introduced the randomized manifest filenames to get some insight into why Josh added it in the first place before deciding to ditch it entirely for the longterm. sstephenson/sprockets@281a3c3#commitcomment-1981744

Owner

leehambley commented Apr 2, 2013

This pull request will be merged today, I'm working through them (46 open!) eek. But I'll get it at least into master this evening.

hardbap commented Apr 2, 2013

I ran into another issue with this. The task fails when you're deploying a new server and there is not an existing manifest file.

hardbap commented Apr 2, 2013

[EDIT]

I deleted my workaround from 8 days ago because it's not a valid fix. It breaks Rails in other exciting ways.

Owner

leehambley commented Apr 2, 2013

Ah, alright - noted, fortunately not merged yet (Github couldn't resolve it automatically) - should I close this issue, and you can reopen when you find something that works across all situations?

Contributor

jimryan commented Apr 2, 2013

I'm happy to push up a fix for that. @hardbap, would you mind posting the error that you receive? I don't have an environment to test this in right now.

Contributor

jimryan commented Apr 2, 2013

In the meantime, @leehambley, as I noted in the PR, rollbacks are broken because Rails 4 doesn't include a assets:precompile:nondigest or equivalent task. I don't know enough about sprockets to comment on why that is. Do all compiled assets now have digests? If so, then we don't need to run that task. Alternatively, we could recompile all assets, but I think that's probably too much work to give the rollback task, which is supposed to be fast.

IMO this is an unrelated issue, so we can move the discussion elsewhere, whatever you think is appropriate.

hardbap commented Apr 2, 2013

Here you go @jimryan:

*** [err :: server2] cp: cannot stat `/home/app-name/shared/assets/manifest-7019565e6588eaa79160a8a8051eb016.json'
*** [err :: server2] : No such file or directory

[EDIT] This is the error message you get when deploying to multiple servers. manifest-7019565e6588eaa79160a8a8051eb016 is the manifest file on server1 but server2 has a manifest with a different digest.

Owner

leehambley commented Apr 2, 2013

Jim, truth be told I'd rather that Rails took some responsibility for their
asset pipeline Capistrano integration, it's such a wildly complicated
system to correctly deploy, I'm not sure we'll ever get it right. If you
can give me a one-word answer about whether I should merge, or close this
pull req I'd appreciate the guidance. I don't use the asset pipeline
myself, so I rely on you guys to tell me whether it's more, or less broken
with this patch applied.

As far as I understand things, this doesn't make anything /correct/, yet.

Please feel free to open an issue, but beware cap v2 is effectively EOL
when Rails 4 ships, we'll continue to support critical fixes, but we have
to move focus to the WIP v3 branch so that problems like this are more
easily and transparently addressed.

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 19:40, Jim Ryan notifications@github.com wrote:

In the meantime, @leehambley https://github.com/leehambley, as I noted
in the PR, rollbacks are broken because Rails 4 doesn't include a
assets:precompile:nondigest or equivalent task. I don't know enough about
sprockets to comment on why that is. Do all compiled assets now have
digests? If so, then we don't need to run that task. Alternatively, we
could recompile all assets, but I think that's probably too much work to
give the rollback task, which is supposed to be fast.

IMO this is an unrelated issue, so we can move the discussion elsewhere,
whatever you think is appropriate.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15790552
.

hardbap commented Apr 2, 2013

@jimryan all compiled assets have a digest in Rails 4.

Contributor

jimryan commented Apr 2, 2013

@leehambley I completely agree.

One word answer: Wait.

I'm going to fix this issue that @hardbap mentioned, and push up a fix.

Rollbacks will still be broken, but I think that they should be solved elsewhere. This will at least get us deploying Rails 4 today.

Contributor

jimryan commented Apr 2, 2013

@hardbap Sweet, thanks. So we just need to check the Rails version to get rollbacks going, we can skip asset regeneration completely for Rails 4.

This is my first time working on capistrano - what's the accepted way to check the Rails version? Or checking the existence of the rake task would work, too.

Owner

leehambley commented Apr 2, 2013

Jim, I'm not sure how you can reliably check the Rails version to be
honest, you might do something around bundle show rails, I think in all
recent versions of Rails, Bundler is a prerequisite (this is one of the
things we're fixing with cap v3, different recipes for different versions
of rails).

I'd also be open to maintaining an rails4 branch of stuff that's not ready
to be merged into the master branch yet, in which case I'd merge this fix
for rails 3, as best we can, and then have something that works for rails 4
on it's own branch, that we could take over into cap v3 when it's ready?

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 19:49, Jim Ryan notifications@github.com wrote:

@hardbap https://github.com/hardbap Sweet thanks. So we just need to
check the Rails version to get rollbacks going, we can skip asset
generation completely for Rails 4.

This is my first time working on capistrano - what's the accepted way to
check the Rails version? Or checking the existence of the rake task would
work, too.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15791080
.

Contributor

jimryan commented Apr 2, 2013

@leehambley Thanks, that should work.

If you'd like to maintain a rails4 branch, that's fine. But as soon as I fix this issue with cold deploys, this will be good to merge except for rollbacks. I can then work on fixing rollbacks and open another PR.

This PR doesn't fix anything for Rails 3, so it should either be merged to master after I push up this other fix, or it should be merged to a rails4 branch right now, and then to master after I submit this other fix. I'd lean towards the former since I can probably get this fix out today and it will be stable at that point, and should be included in the gem as soon as possible, since Rails 4 is in beta and this is only going to become an issue for more people.

Owner

leehambley commented Apr 2, 2013

Jim, cheers - so this works well enough for Rails 3 as well? I'm totally
lost with all the movement in the asset pipeline currently, especially the
wild changes to the Gemfile and :assets groups with the Rails 4 HEAD today.

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 20:04, Jim Ryan notifications@github.com wrote:

@leehambley https://github.com/leehambley Thanks, that should work.

If you'd like to maintain a rails4 branch, that's fine. But as soon as I
fix this issue with cold deploys, this will be good to merge except for
rollbacks. I can then work on fixing rollbacks and open another PR.

This PR doesn't fix anything for Rails 3, so it should either be merged to
master after I push up this other fix, or it should be merged to a rails4
branch right now, and then to master after I submit this other fix. I'd
lean towards the former since I can probably get this fix out today and it
will be stable at that point, and should be included in the gem as soon as
possible, since Rails 4 is in beta and this is only going to become an
issue for more people.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15792018
.

Contributor

jimryan commented Apr 2, 2013

As far as I know, there was no problem with Rails 3. It looks like Rails 3 writes its own manifest, which is still in YAML. Rails 4 uses sprockets-rails which lets sprockets write the manifest, which (now) writes a JSON manifest with a random filename.

I can confirm that capistrano works just fine on my Rails 3 projects with and without this commit.

Owner

leehambley commented Apr 2, 2013

Alright, awesome - so as long as this commit doesn't make things worse on
Rails 3, I'm more than happy to merge it, just give me the word

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 20:23, Jim Ryan notifications@github.com wrote:

As far as I know, there was no problem with Rails 3. It looks like Rails 3
writes its own manifest, which is still in YAML. Rails 4 uses
sprockets-rails which lets sprockets write the manifest, which (now) writes
a JSON manifest with a random filename.

I can confirm that capistrano works just fine on my Rails 3 projects with
and without this commit.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15793078
.

Contributor

jimryan commented Apr 2, 2013

Great, I'll ping you when I've fixed the issue that Mike brought up and ensure that it's fixed on Rails 3 and 4.

Owner

leehambley commented Apr 2, 2013

Great, thanks a lot!

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 20:35, Jim Ryan notifications@github.com wrote:

Great, I'll ping you when I've fixed the issue that Mike brought up and
ensure that it's fixed on Rails 3 and 4.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15793859
.

hardbap commented Apr 2, 2013

@jimryan The error message I pasted earlier is actually when you are deploying to multiple servers not the case of deploying cold. Here is the actual error I get when deploying cold (there is no existing manifest file):

* 2013-04-02 14:41:12 executing `deploy:assets:precompile'
triggering before callbacks for `deploy:assets:precompile'
* 2013-04-02 14:41:12 executing `deploy:assets:update_asset_mtimes'
* executing "ls /home/app-name/shared/assets/manifest*"
servers: ["server1"]
[server1] executing command
[err :: server1] ls: cannot access /home/app-name/shared/assets/manifest*
[err :: server1] : No such file or directory

Sorry for the confusion.

Contributor

jimryan commented Apr 2, 2013

@hardbap Thanks for the clarification. There's definitely two issues here.

Forgive my ignorance, I haven't deployed with capistrano to multiple servers. What's the expected behavior in terms of asset compilation? Should the assets compile on each server (which seems horribly inefficient) - or are they compiled and then somehow transferred to the additional servers?

@leehambley leehambley referenced this pull request Apr 2, 2013

Closed

Rails 4 support #362

Owner

leehambley commented Apr 2, 2013

AFAIK the expected behaviour is to compile them on every :app role (yes,
horrible, but I'd still recommend people precompile and commit their
assets!)

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 2 April 2013 21:07, Jim Ryan notifications@github.com wrote:

@hardbap https://github.com/hardbap Thanks for the clarification.
There's definitely two issues here.

Forgive my ignorance, I haven't deployed with capistrano to multiple
servers. What's the expected behavior in terms of asset compilation? Should
the assets compile on each server (which seems horribly inefficient) - or
are they compiled and then somehow transferred to the additional servers?


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-15795823
.

Contributor

jimryan commented Apr 2, 2013

@leehambley Yeah, I'm not really sure what the benefits are of compiling on the server instead of committing the compiled assets (even though that's what I do), other than a reduction in repo size, for whatever that's worth.

Alright, so I guess we'll need to grab the manifest filename for each host, since each server's manifest will have a completely different filename. I'll need to do a bit more research into how capistrano deals with multi-server deploys. I don't expect to get this out today, but I'll work on something as quickly as possible and let you know when it's ready.

Owner

leehambley commented Apr 3, 2013

Alright, so I guess we'll need to grab the manifest filename for each host, since each server's manifest will have a completely different filename. I'll need to do a bit more research into how capistrano deals with multi-server deploys. I don't expect to get this out today, but I'll work on something as quickly as possible and let you know when it's ready.

That's not really possible, the block is executed as-is on each host that's the reason a lot of things resort to nasty bash scripting to let the ruby block look the same everywhere, and server specific things happen according to the environment in which bash is running on the box.

This is one of the principle reasons for the changes in Cap3 (where the block can take different paths depending on the host it's on, and answers such as captures() from the server)

Contributor

jimryan commented Apr 11, 2013

I just added some commits to this PR. I believe that this resolves the outstanding issues. This should now support:

  • Asset manifests with randomized filenames in multi-server environments. As part of the assets:precompile task, I sync the manifest filenames across all servers with whatever the name is on the first server.
  • Cold deploys. We make no assumptions about the existence of the manifest file until after the assets:precompile task, at which point it's safe to assume that it exists and has the same name on all servers.
  • Rollbacks in Rails 4. Rather than checking the Rails version, I check if the assets:precompile:nondigest rake task exists. If it doesn't, we assume that there are no nondigest assets to compile and move on.

@hardbap Would you mind testing this to make sure that this works in your multi-server environment? I don't have access to one and haven't had time to set one up locally. I'm fairly certain that it's resolved, but would like to confirm before asking that this be merged.

Owner

leehambley commented Apr 15, 2013

Great work guys, coupel of things:

@jimryan can you rebase this against master so that it'll merge cleanly?

Everyone else, if you could test this, then I'll be more than happy to merge it!

Contributor

jimryan commented Apr 15, 2013

@leehambley Done. I've tested deploys and rollbacks on a single server Rails 4 app, and it's still working beautifully. I'm a little concerned about #427, but left it in there for now. I voiced my concerns over there (#427 (comment)).

I'd just wait on @hardbap (or anyone else) to test this in a multi-server environment before merging.

Owner

leehambley commented Apr 15, 2013

Great work Jim, acknowledged about #427 - I also need to think about that a
little more, I didn't know if we'd broken that sometime. But in the months
I wasn't maintaining, something may have happened.

Lee Hambley

http://lee.hambley.name/
+49 (0) 170 298 5667

On 15 April 2013 19:21, Jim Ryan notifications@github.com wrote:

@leehambley https://github.com/leehambley Done. I've tested deploys and
rollbacks on a single server Rails 4 app, and it's still working
beautifully. I'm a little concerned about #427#427,
but left it in there for now. I voiced my concerns over there (#427#427 (comment)
).

I'd just wait on @hardbap https://github.com/hardbap (or anyone else)
to test this in a multi-server environment before merging this.


Reply to this email directly or view it on GitHubhttps://github.com/capistrano/capistrano/pull/412#issuecomment-16398890
.

Contributor

jimryan commented Apr 15, 2013

Thanks! I'm not really sure what the issue was. That fix looks more like a bandaid to me, the actual problem is probably that we don't have a current_release path. If we don't copy the manifest to the release path, then after :expire_assets_after (default is 1 week), the assets for that deploy will be removed. This probably isn't a huge problem, because rollbacks would also be broken without that backed up manifest, so that release would never be usable again anyway.

korny commented Apr 20, 2013

Thank you, this is so helpful. I also agree that it should not be Capistrano's responsibility, but rather Sprocket's.

In an ideal world, we would just add:

require 'sprockets/capistrano'

to the deploy.rb to get an up-to-date assets deploy task.

Contributor

jimryan commented Apr 21, 2013

I tend to agree, but I'm not sure that any extra work should be required to get capistrano working with sprockets. If capistrano is supposed to work OOTB with Rails, then requiring additional work would probably confuse people using sprockets with Rails, which is the default behavior.

I do think that breaking sprockets support out into its own gem might help spur some contribution from people who work on sprockets. Even though that sort of contradicts what I've said above.

Owner

leehambley commented Apr 23, 2013

@jimryan Can you rebase this so that it'll apply cleanly? (I'm trying to smash my way through ~35 or so new emails related to Capistrano in the last few days, and I'd love to merge this, but I can't afford time to resolve merge conflicts)

Contributor

jimryan commented Apr 23, 2013

@leehambley You're all set. I'd consider this good to merge, although I would have preferred someone confirm it working in a multi-server environment. There shouldn't be any changes that impact < Rails 4 multi-server environments, so I don't think that this would cause any regressions, the worst case is it continues to not work for some Rails 4 users instead of all Rails 4 users. Although I do believe I have resolved the multi-server issues.

@leehambley leehambley added a commit that referenced this pull request Apr 23, 2013

@leehambley leehambley Merge pull request #412 from jimryan/support-json-manifest
Support JSON manifests and manifests with randomized filenames.
4bc2b6b

@leehambley leehambley merged commit 4bc2b6b into capistrano:master Apr 23, 2013

1 check passed

default The Travis build passed
Details
Contributor

jimryan commented Apr 23, 2013

Thank you!

@jimryan jimryan deleted the jimryan:support-json-manifest branch Apr 23, 2013

Owner

leehambley commented Apr 23, 2013

Great work @jimryan. I pushed this in 2.15.1 so that anyone who's having problems can take 2.15.0 without this change, but most people should just get the new hotness. They both went to Rubygems concurrently about 2 minutes ago, so they should start showing soon enough.

Contributor

jimryan commented Apr 23, 2013

Fantastic! I'll try to keep an eye on the issues in case anyone experiences any problems with anything asset-related. Feel free to ping me on those issues.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment