Avoid shelling out for generator generate action

[jonathanhefner]

This change addresses a few issues:

First, shelling out to the rails command requires the destination directory to contain certain files that the command uses to initialize itself. While this is not an issue when running generators normally, it is troublesome when testing generator-invoking generators which output to ephemeral destination directories.

Second, shelling out to the rails command is very slow. This also is not a particular concern when running generators normally, but it makes test suites for generator-invoking generators painfully slow.

Third, shelling out to the rails command fails silently by default. Such silent failures can be surprising, and can lead to confusing downstream failures.

---

Some previous history regarding the generate action:

• Allow usage of the generate action in generators when testing #34418 was created to prevent generator sub-invocations from failing silently. It was rejected due to concerns about backward compatibility.
• Abort early if generator command fails #34420 was created to solve a generalized form of the issue addressed by Allow usage of the generate action in generators when testing #34418, but the solution is opt-in.
• Don't add RAILS_ENV in generate action #34980 was created to fix a regression introduced by Abort early if generator command fails #34420, specific to generate.

The changes in this PR are similar to #34418, but with backward-compatibility concerns mostly addressed. However, unlike the alternative #34420, generate will now always raise on failure. This could be made opt-in behavior, but I could not think of a scenario where you would want generate to fail silently.

This PR does not revert #34420, which affected more than generate, but this PR does obsolete the abort_on_failure option for generate. Since abort_on_failure was previously only tested with generate, this PR adds tests for the abort_on_failure option with other actions.

This PR does revert #34980, which is no longer necessary. (See #37979)

Also, a crude and informal performance measurement: for a test suite with fewer than a dozen generator-invoking-generator tests, this change reduced the run time from 57 seconds to 3.3 seconds. 🚀

[jonathanhefner]

The build errors are coming from Active Job. I will take a closer look tomorrow.

[jonathanhefner]

While digging into the Active Job errors, I realized a use case which does require that generate shell out. Imagine two consecutive generate calls where the 1st call generates app config code that the 2nd depends on. Without shelling out, Rails.application is not "reloaded" for the 2nd call, and thus the generated app config code will not take effect.

The benefits of not shelling out are still very desirable, though. My current thinking is to at least add a :spawn option to generate (or possibly to rails_command). It could default to true, but when set to false, the action would use Rails::Command.invoke instead of execute_command :rails.

Thoughts, anyone?

[deivid-rodriguez]

Third, shelling out to the rails command fails silently by default. Such silent failures can be surprising, and can lead to confusing downstream failures.

I think this third issue would be solved by rails/thor#651?

[kaspth]

❤️

[kaspth]

Why are we inlining this?

[jonathanhefner]

I'm not sure I understand the question. Are you referring to the concerns you brought up in #34980 (comment)?

[kaspth]

While digging into the Active Job errors, I realized a use case which does require that generate shell out. Imagine two consecutive generate calls where the 1st call generates app config code that the 2nd depends on. Without shelling out, Rails.application is not "reloaded" for the 2nd call, and thus the generated app config code will not take effect.

Is there a way to make Rails.application reload itself? Or what's the stuff that isn't reloaded properly? Do we just not have the appropriate reloaders in place to pick up e.g. app/models/post.rb was in app/models because app/models was created simultaneously?

[jonathanhefner]

Build errors are now unrelated. (They've been happening since ace8d6d, though I suspect the cause is some upstream dependency.)

[jonathanhefner]

@kaspth

Is there a way to make Rails.application reload itself? Or what's the stuff that isn't reloaded properly? Do we just not have the appropriate reloaders in place to pick up e.g. app/models/post.rb was in app/models because app/models was created simultaneously?

There does not seem to be a way to have Rails reload app config code, e.g. config/application.rb. So (quite) unfortunately this new behavior may have to be opt-in after all, to preserve backward compatibility.

My current approach is to add an :inline option to rails_command, which generate now delegates to. When inline: true, rails_command will call Rails::Command.invoke directly in the current process, instead of shelling out.

[jonathanhefner]

@deivid-rodriguez

I think this third issue would be solved by erikhuda/thor#651?

If I understand that PR correctly, exit_on_failure? will default to false, so shelling out would still fail silently by default. It would be up to each generator class to override exit_on_failure? and return true, correct?

However, I did add a change to this PR so that generate always sets abort_on_failure: true. So now calls to generate will always raise on failure. What do you think?

[deivid-rodriguez]

I'm not sure. Rails does this:

rails/railties/lib/rails/commands/application/application_command.rb

Lines 6 to 16 in b2eb1d1

	module Rails
	module Generators
	class AppGenerator # :nodoc:
	# We want to exit on failure to be kind to other libraries
	# This is only when accessing via CLI
	def self.exit_on_failure?
	true
	end
	end
	end

It would seem to me that the expected behaviour is to actually not raise in some cases, but in others ("when accessing via CLI"), the thor PR would fix the issue?

[jonathanhefner]

@deivid-rodriguez I think rails/thor#651 is intended to address the case where run is never allowed to fail, inside a single generator class (and its subclasses). But the case I'm trying to address is, more specifically, generate is never allowed to fail, for all generator classes.

[kaspth]

Perfect @jonathanhefner, thanks for preserving backwardscompatibilty too. What's the next step to get this speed boost? Is it mostly for our own generator tests?

[jonathanhefner]

@kaspth Any generator which invokes the rails_command or generate actions can opt in to the speed boost with inline: true. Looking through the "meta-generators" in Rails core, e.g. ScaffoldGenerator and ControllerGenerator, all of them appear to use hook_for rather than generate. However, there are a few generators which use rails_command. I've created #38429 to add inline: true to those, but I've had difficulty judging the performance improvement (see my comments there).