Skip to content
New issue

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

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove unused dependencies bigdecimal and activesupport #504

Merged
merged 3 commits into from
Jul 4, 2023

Conversation

crazytonyli
Copy link
Contributor

What does it do?

I don't know much about the history of the two removed dependencies bigdecimal and activesupport, but they appear to not being used by this library.

There was this #289 issue documenting a version clash between this library and some app's gems. But I can't find what the app was, and don't know if that issue is still relevant. I'm happy to dig deeper if someone can provide more context around these two dependencies. Thanks!

Checklist before requesting a review

  • Run bundle exec rubocop to test for code style violations and recommendations
  • Add Unit Tests (aka specs/*_spec.rb) if applicable
  • Run bundle exec rspec to run the whole test suite and ensure all your tests pass
  • Make sure you added an entry in the CHANGELOG.md file to describe your changes under the appropriate existing ### subsection of the existing ## Trunk section.
  • If applicable, add an entry in the MIGRATION.md file to describe how the changes will affect the migration from the previous major version and what the clients will need to change and consider.

@crazytonyli crazytonyli requested a review from a team July 3, 2023 22:01
jkmassel
jkmassel previously approved these changes Jul 3, 2023
Copy link
Contributor

@jkmassel jkmassel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is ok, but in the past we'd included these in the gemspec because we needed a minimum version of both.

IMHO to accommodate that, we should pin the release toolkit to a more recent version of fastlane, which itself specifies these for us?

WDYT?

@mokagio
Copy link
Contributor

mokagio commented Jul 4, 2023

@jkmassel

IMHO to accommodate that, we should pin the release toolkit to a more recent version of fastlane, which itself specifies these for us?

See also #482

@mokagio
Copy link
Contributor

mokagio commented Jul 4, 2023

There was this #289 issue documenting a version clash between this library and some app's gems. But I can't find what the app was, and don't know if that issue is still relevant.

The app was WordPress iOS and the issue had to do with CocoaPods.

From what I can gather from the issue – I had forgotten all about it – the problem had to do with CocoaPods depending on activesupport which in turn depended on bigdecimal. Looks like the bigdecimal dependency is no longer there which makes me quite confident this removal will be fine.

Still, I triggered a WordPress iOS build with this branch in CI to verify. bundle install worked on my machine ✅ 😅

Update: The build failed, but the reason was the breaking change from #445. bundle install worked fine 👍

@crazytonyli
Copy link
Contributor Author

@jkmassel @mokagio I don't see any issue in declaring fastlane as a dependency. Added in 161dec9.

@crazytonyli crazytonyli dismissed jkmassel’s stale review July 4, 2023 02:39

Needs another look

@crazytonyli crazytonyli merged commit 4e11a60 into trunk Jul 4, 2023
7 checks passed
@crazytonyli crazytonyli deleted the remove-unused-dependencies branch July 4, 2023 04:38
@crazytonyli crazytonyli mentioned this pull request Jul 4, 2023
5 tasks
@AliSoftware
Copy link
Contributor

I just spend 45mn trying to understand why fastlane couldn't find my actions while I was trying to test integrating my new actions from #505 into Tumblr-Android 😓

At first I thought it was an issue with the actions I just added so I tried to bisect my changes… but finally I ended up looking at previous changes then narrowing it down to this PR being the culprit.

The issue is that if you point your Gemfile to any commit of release-toolkit that is past 4e11a60 (the commit corresponding to the merge of this PR into trunk), you will get the following error for any fastlane command:

[✔] 🚀 
[16:56:21]: Error loading plugin 'fastlane-plugin-wpmreleasetoolkit': cannot load such file -- active_support/all
+-----------------------------------+---------+---------------------------------------------------------------------------+
|                                                      Used plugins                                                       |
+-----------------------------------+---------+---------------------------------------------------------------------------+
| Plugin                            | Version | Action                                                                    |
+-----------------------------------+---------+---------------------------------------------------------------------------+
| fastlane-plugin-appcenter         | 1.11.0  | appcenter_fetch_version_number, appcenter_fetch_devices, appcenter_upload |
| fastlane-plugin-wpmreleasetoolkit | 8.1.0   | No actions found                                                          |
+-----------------------------------+---------+---------------------------------------------------------------------------+

[!] No actions were found while loading one or more plugins
    Please use `bundle exec fastlane` with plugins
    More info - https://docs.fastlane.tools/plugins/using-plugins/#run-with-plugins

I think we need to revert at the very least the removal of activesupport (i.e. re-add it to the plugin's dependencies) to be able to make it work again. @crazytonyli I'm just hoping that removal of activesupport that was done in this PR wasn't a requirement for the toolkit to support Ruby 3.2.2 or something?

@crazytonyli
Copy link
Contributor Author

crazytonyli commented Jul 19, 2023

Ah sorry. I think I may have used searched the wrong thing when searching for code that uses activetsupport. I probably searched activesupport, not active_support...

I'm just hoping that removal of activesupport that was done in this PR wasn't a requirement for the toolkit to support Ruby 3.2.2 or something?

I think removing bigdecimal is a requirement, but not activesupport. I removed activesupport because the issue that I looked into (#289) updated both. I thought these two gems may have some sort of connection.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Version clash for the activesupport & bigdecimal gems between toolkit and cocoapods-core
4 participants