Shopify / deprecation_toolkit

⚒Eliminate deprecations from your codebase ⚒
MIT License
454 stars 40 forks source link

Drop support for EOL versions & test against newer ones #58

Closed sambostock closed 2 years ago

sambostock commented 2 years ago

The most important changes made in this PR are that it drops support for the following:

Several related changes are made:

sambostock commented 2 years ago

This failure seems to occur on only the Active Support 7.0 CI runs, but be fixed in the edge runs.

...activesupport-7.0.2.2/lib/active_support/xml_mini.rb:184:in `current_thread_backend':
   uninitialized constant ActiveSupport::XmlMini::IsolatedExecutionState (NameError)

        IsolatedExecutionState[:xml_mini_backend]

Using git bisect on rails/rails, I've identified https://github.com/rails/rails/pull/44340 as fixing it. However, that PR mentions

For better or worse, this change will incidentally fix the instances of people requiring some individual files from Active Support without first loading active_support.rb that, while not officially supported, were previously working and recently started failing. (https://github.com/rails/rails/issues/43851 https://github.com/rails/rails/pull/43852 https://github.com/rails/rails/issues/43889 https://github.com/rails/rails/issues/43908 https://github.com/rails/rails/issues/44336) This is not a reversal of that supported-usage policy, and a future change could re-break it at any time.

Based on that, we must have a usage of an Active Support internal file without require "active_support" first. It would probably make sense to track it down and address it before merging this, rather than skipping the 7.0 entries in the test matrix, but that is always an option. Note that (as far as I can tell), this is revealing an existing bug, not introducing one (though I may be wrong).

sambostock commented 2 years ago

The issue is that #50 added https://github.com/Shopify/deprecation_toolkit/blob/0bfe3782acff87e74ce7ff17792de53ce8b98e18/lib/deprecation_toolkit.rb#L38 near the bottom of the file, whereas if needs to be the first thing we do, before even https://github.com/Shopify/deprecation_toolkit/blob/0bfe3782acff87e74ce7ff17792de53ce8b98e18/lib/deprecation_toolkit.rb#L3 because the task code includes https://github.com/Shopify/deprecation_toolkit/blob/0bfe3782acff87e74ce7ff17792de53ce8b98e18/lib/tasks/ci_recorder.rake#L5 and as described in my previous comment, requiring pieces of Active Support before requiring the main file first is unsupported behaviour.