Add forced_traits as an option to Config.new

[marcheiligers]

Implements #587

[headius]

@kares @chadlwilson Can you take a look at this? I think we're due for a full refactoring of how we configure (and detect configuration) of the resulting war/jar file, but this might be a reasonable half-step to allow jarring up applications that would otherwise be detected as a rack-able war file.

[chadlwilson]

Thanks for the work here @marcheiligers !

I'm a bit concerned that by introducing conflicts, the relationship between detect?, requirements and conflicts has become even more confusing to reason about, since it seems only to apply when forcing traits? Right now, conflicts is somewhat implicitly defined within detect? priority in several places.

Perhaps we should go through all the traits and see how to model that consistently to ensure it makes sense?
e.g:
nogemspec conflicts with gemspec

warbler/lib/warbler/traits/nogemspec.rb

Line 19 in 62bbd62

Jar.detect? && !Gemspec.detect?

rack conflicts with rails

warbler/lib/warbler/traits/rack.rb

Line 15 in 0747bcf

!Rails.detect? && (File.exist?("config.ru") || !Dir['*/config.ru'].empty?)

...etc

"Force" style

With this approach, if I am correct, it seems you can forcibly enable a trait; e.g to resolve a conflict (executable jar vs war) but you cannot forcibly disable a trait which is otherwise detected but unwanted?

For example, the use case at #587 (comment) where Traits::Gemspec starts getting enabled just because there is a .gemspec file that exists.

One could argue that with your approach we would just define a conflict between gemspec and nogemspec and the user could force enable NoGemspec. OK, but perhaps there are other cases where there may be no natural conflict and the detection is just wrong, and I want to disable it by specifying an entire list of traits?

In other words, the challenge perhaps is not solely being able to deal with conflicts between traits where the detection is (arguably) indeterminate, but traits which you do not want to be detected at all?

Would it be simpler to

• just take the forced_traits as-is and expect a complete set if non-empty
• disable all auto-detection if it is non-empty, and instead
  • tsort it (for application order) by requirements as it currently does (don't expect users to get this write, not allow them to change that order)
  • model the conflicts (as you've started to) and fail fast if conflicts are detected
    • ideally we could reuse that conflicts modelling in the existing detect? logic so it's clearer how it relates e.g:
      

      warbler/lib/warbler/traits/nogemspec.rb

      Line 19 in 62bbd62

      Jar.detect? && !Gemspec.detect?

      
      

      warbler/lib/warbler/traits/rack.rb

      Line 15 in 0747bcf

      !Rails.detect? && (File.exist?("config.ru") || !Dir['*/config.ru'].empty?)

      
      ...etc

Other thoughts

• we probably should document this somewhere; and also note that if you force enable some traits you may break things completely; since this bypasses all the pre-requisites detection

[chadlwilson]

Any idea why this attribute is also in the Traits super? Do we need to change both? (i'm not an expert with this particular Ruby magic)

warbler/lib/warbler/traits.rb

Line 18 in cc3bce2

attr_accessor :traits

[marcheiligers]

I actually removed that other one in this PR, since the Traits module never uses it. Config includes Trait, and having both (previously attr_accessor :traits) doesn't do any harm, but is redundant.

I changed it to attr_reader so you cannot assign a new array of Traits in Config, but actually it doesn't prevent you from manipulating the existing array. This is a way that one could remove an unwanted Trait (config.traits.reject! { |t| t.is_a?(Warbler::Traits::Gemspec) }) but doing so does not remove the config changes that that Trait will already have made in before_configure. (I'm just thinking out loud here).

[chadlwilson]

Bit of a nitpick, but perhaps we don't need to store the forced_traits and can just pass them to auto_detect_traits for consideration, and continue to store only the "resolved" traits. Avoids the coupling ion the instance vars between calls and makes it a bit more obvious that auto_detect_traits is not solely auto detection - without have to rename the method and potentially breaking any overrides/monkey-patches.

[marcheiligers]

I did both during development 🤣. In the end I decided to assign it to an instance var in case it became useful to know what was forced later. I'm not married to it.

[marcheiligers]

Thanks for the review, @chadlwilson

I'm a bit concerned that by introducing conflicts, the relationship between detect?, requirements and conflicts has become even more confusing to reason about, since it seems only to apply when forcing traits? Right now, conflicts is somewhat implicitly defined within detect? priority in several places.

Agreed on the concern, but I needed a way to know that if you're forcing it to be a Jar, then it can't also be a War, which would've detect?ed

Perhaps we should go through all the traits and see how to model that consistently to ensure it makes sense?

The table below represents the current state. Italic Conflicts have not been introduced in this PR, but would seem to be the obvious ones that make sense.

With this approach, if I am correct, it seems you can forcibly enable a trait; e.g to resolve a conflict (executable jar vs war) but you cannot forcibly disable a trait which is otherwise detected but unwanted?

For example, the use case at #587 (comment) where Traits::Gemspec starts getting enabled just because there is a .gemspec file that exists.

One could argue that with your approach we would just define a conflict between gemspec and nogemspec and the user could force enable NoGemspec. OK, but perhaps there are other cases where there may be no natural conflict and the detection is just wrong, and I want to disable it by specifying an entire list of traits?

Correct on not being able to remove an unwanted trait.

Also, NoGemspec is not the opposite of Gemspec ... a (normal) Rails War has neither Gemspec nor NoGemspec traits. So, does Rails conflict with Gemspec and NoGemspec? This seems to indicate that it does, but I'm not sure. Forcing NoGemspec would probably work, but would result in a slightly different outcome than what they're doing now which has neither Trait. I'll have to experiment with this.

just take the forced_traits as-is and expect a complete set if non-empty

I had considered this and decided against it as it would require the end-user to know the full set of traits needed. For @headius' Sidekiq example, that would be [Warbler::Traits::Jar, Warbler::Traits::Gemspec, Warbler::Traits::Bundler] instead of just [Warbler::Traits::Jar] with the rest autodetected. I think this provides a better experience, but I could be convinced otherwise (with sufficient documentation).

ideally we could reuse that conflicts modelling in the existing detect? logic so it's clearer how it relates

That would be neat, but I think these concerns are orthogonal. detect? is saying "if this condition is met, then it has this Trait" (in the general sense ... specifically it is looking for the existence of file(s), but it could use any other logic, like looking in the Gemfile to see if the Rails gem is in there). conflicts is saying "if it has this Trait, it cannot also have these other Traits".

It could should definitely fail fast if you tried to Force conflicting Traits, which it doesn't now.

we probably should document this somewhere; and also note that if you force enable some traits you may break things completely; since this bypasses all the pre-requisites detection

It's not bypassing prerequisites detection. It's just removing conflicts if you force a particular trait. The rest of the auto-detection still happens. Agreed on documentation (and failing fast) though.

---

Trait	Requirements	Conflicts	Auto-detect condition
Jar	—	War	!War.detect?
War	—	Jar	Rails.detect? || Rack.detect?
Rails	War	Rack	File.exist?('config/environment.rb')
Rack	War	Rails	!Rails.detect? && (File.exist?("config.ru") || !Dir['*/config.ru'].empty?)
Bundler	War, Jar	—	File.exist?(ENV['BUNDLE_GEMFILE'] || 'Gemfile')
JBundler	War, Jar	—	File.exist?(ENV['JBUNDLE_JARFILE'] || "Jarfile")
Gemspec	—	NoGemspec	!Dir['*.gemspec'].empty?
NoGemspec	—	Gemspec	Jar.detect? && !Gemspec.detect?

[marcheiligers]

I've been noodling on the #587 (comment) problem and how it relates to the issue @headius raised.

The solution presented in this PR (forcing traits) does not solve the issue in the comment (rejecting a trait). A solution allowing for rejecting (or ignoring) Traits would solve the issue from the comment, but not the original issue (rejecting Warbler::Traits::War does not result in Warbler::Traits::Jar being detected).

One solution that would work has been suggested above is that the forced_traits is required to be a full list of Traits, effectively bypassing auto-detection entirely, at the cost of the user needing to understand which Traits they need.

The other option is that we provide a second argument reject_traits or ignore_traits to remove unwanted Traits. I'm not convinced that this is the correct way to go.

I'll create a second PR to see what the "full list of Traits" solution looks like.

[chadlwilson]

Really? I haven't actually tried it yet, but I thought the way this worked for the specific case was by forcing trait x it causes it to reject all traits that are defined to conflict, which serves the need of opting out of war detection when you just want a regular jar and no servlet container?

(I'll come back to the longer message in a bit)