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

[WIP] Require relative all the things #13474

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

milch
Copy link
Collaborator

@milch milch commented Oct 1, 2018

Supersedes #11507

So since I never got around to completing that PR, and rebasing onto master turned out not so well, I just did some magic code copying around from the old branch and re-ran the rubocop I added here.
Did some fix ups as well after, and some smoke testing, which succeeded.

The reason this is still WIP is because

  1. I'd want to do some more testing. If someone has some test projects I'd appreciate if you could run this branch on that and report back with the results. Unfortunately I don't have a test project handy at the moment that I could use
  2. Some work needs to be done to make the Require/MissingRequireStatement rubocop work with the internal references

@fastlane-bot-helper
Copy link
Contributor

2 Warnings
⚠️ Big PR
⚠️ Pull Request is Work in Progress

Generated by 🚫 Danger

@milch
Copy link
Collaborator Author

milch commented Oct 1, 2018

Thank you kind Mr. Bot

@milch milch mentioned this pull request Oct 1, 2018
4 tasks
@googlebot
Copy link

So there's good news and bad news.

👍 The good news is that everyone that needs to sign a CLA (the pull request submitter and all commit authors) have done so. Everything is all good there.

😕 The bad news is that it appears that one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that here in the pull request.

Note to project maintainer: This is a terminal state, meaning the cla/google commit status will not change from this state. It's up to you to confirm consent of all the commit author(s), set the cla label to yes (if enabled on your project), and then merge this pull request when appropriate.

@googlebot googlebot added cla: no and removed cla: yes labels Oct 1, 2018
@janpio
Copy link
Member

janpio commented Oct 1, 2018

CI tests are now succeeding again after I pushed a tiny bug fix to your branch.

Will now test it with https://github.com/janpio/fastlane-manual-integration-tests

@janpio
Copy link
Member

janpio commented Oct 2, 2018

Tested on Windows for typical Android workflow:

fastlane supply init
fastlane screengrab init
gradle
build_android_app
copy_artifacts
fastlane screengrab
capture_android_screenshots
copy_artifacts
fastlane supply
upload_to_play_store

Works as expected.

@KrauseFx
Copy link
Member

KrauseFx commented Oct 2, 2018

I'm extremely excited about this PR 👍 Thanks @milch for all your hard work on this

@janpio
Copy link
Member

janpio commented Oct 2, 2018

Do you still see the impressive speed improvements from the original PR @milch?

On CI it seems this branch is a tiny-tiny bit slower than current master.
Of course this doesn't matter as the code is so much better - but I wonder where the improvement went to that you observed on the original PR :/

comparison of `benchmark_help_command` lane for this branch and current `master`

require-relative-best-require:

Ubuntu

[09:02:25]: ---------------------------
[09:02:25]: --- Step: shell command ---
[09:02:25]: ---------------------------
[09:02:27]: 🏎️ 'Total   2.0641s'
[09:02:27]: ---------------------------
[09:02:27]: --- Step: shell command ---
[09:02:27]: ---------------------------
[09:02:30]: 🏎️ 'Total   2.1153s'
[09:02:30]: ---------------------------
[09:02:30]: --- Step: shell command ---
[09:02:30]: ---------------------------
[09:02:32]: 🏎️ 'Total   2.1212s'
[09:02:32]: Turning on FASTLANE_OPTIMIZE_BOOT:
[09:02:32]: ---------------------------
[09:02:32]: --- Step: shell command ---
[09:02:32]: ---------------------------
[09:02:34]: 🏎️ 'Total   2.0655s'
[09:02:34]: ---------------------------
[09:02:34]: --- Step: shell command ---
[09:02:34]: ---------------------------
[09:02:35]: 🏎️ 'Total   0.7875s'
[09:02:35]: ---------------------------
[09:02:35]: --- Step: shell command ---
[09:02:35]: ---------------------------
[09:02:36]: 🏎️ 'Total   0.7456s'
[09:02:36]: Turning off FASTLANE_OPTIMIZE_BOOT again.

macOS

[02:03:34]: ---------------------------
[02:03:34]: --- Step: shell command ---
[02:03:34]: ---------------------------
[02:03:35]: 🏎️ 'Total   1.1140s'
[02:03:35]: ---------------------------
[02:03:35]: --- Step: shell command ---
[02:03:35]: ---------------------------
[02:03:36]: 🏎️ 'Total   1.0749s'
[02:03:36]: ---------------------------
[02:03:36]: --- Step: shell command ---
[02:03:36]: ---------------------------
[02:03:37]: 🏎️ 'Total   1.0862s'
[02:03:37]: Turning on FASTLANE_OPTIMIZE_BOOT:
[02:03:37]: ---------------------------
[02:03:37]: --- Step: shell command ---
[02:03:37]: ---------------------------
[02:03:39]: 🏎️ 'Total   1.5589s'
[02:03:39]: ---------------------------
[02:03:39]: --- Step: shell command ---
[02:03:39]: ---------------------------
[02:03:40]: 🏎️ 'Total   0.5630s'
[02:03:40]: ---------------------------
[02:03:40]: --- Step: shell command ---
[02:03:40]: ---------------------------
[02:03:41]: 🏎️ 'Total   0.5470s'
[02:03:41]: Turning off FASTLANE_OPTIMIZE_BOOT again.

Windows

[09:03:36]: ---------------------------
[09:03:36]: --- Step: shell command ---
[09:03:36]: ---------------------------
[09:03:38]: 🏎️ 'Total   2.3592s'
[09:03:38]: ---------------------------
[09:03:38]: --- Step: shell command ---
[09:03:38]: ---------------------------
[09:03:41]: 🏎️ 'Total   2.4558s'
[09:03:41]: ---------------------------
[09:03:41]: --- Step: shell command ---
[09:03:41]: ---------------------------
[09:03:44]: 🏎️ 'Total   2.3480s'
[09:03:44]: Turning on FASTLANE_OPTIMIZE_BOOT:
[09:03:44]: ---------------------------
[09:03:44]: --- Step: shell command ---
[09:03:44]: ---------------------------
[09:03:47]: 🏎️ 'Total   3.3008s'
[09:03:47]: ---------------------------
[09:03:47]: --- Step: shell command ---
[09:03:47]: ---------------------------
[09:03:50]: 🏎️ 'Total   2.1915s'
[09:03:50]: ---------------------------
[09:03:50]: --- Step: shell command ---
[09:03:50]: ---------------------------
[09:03:52]: 🏎️ 'Total   2.0913s'
[09:03:52]: Turning off FASTLANE_OPTIMIZE_BOOT again.

master:

Ubuntu

[20:38:28]: ---------------------------
[20:38:28]: --- Step: shell command ---
[20:38:28]: ---------------------------
[20:38:31]: 🏎️ 'Total   2.0625s'
[20:38:31]: ---------------------------
[20:38:31]: --- Step: shell command ---
[20:38:31]: ---------------------------
[20:38:33]: 🏎️ 'Total   2.1198s'
[20:38:33]: ---------------------------
[20:38:33]: --- Step: shell command ---
[20:38:33]: ---------------------------
[20:38:35]: 🏎️ 'Total   2.2003s'
[20:38:35]: Turning on FASTLANE_OPTIMIZE_BOOT:
[20:38:35]: ---------------------------
[20:38:35]: --- Step: shell command ---
[20:38:35]: ---------------------------
[20:38:38]: 🏎️ 'Total   2.1139s'
[20:38:38]: ---------------------------
[20:38:38]: --- Step: shell command ---
[20:38:38]: ---------------------------
[20:38:39]: 🏎️ 'Total   0.7423s'
[20:38:39]: ---------------------------
[20:38:39]: --- Step: shell command ---
[20:38:39]: ---------------------------
[20:38:40]: 🏎️ 'Total   0.6707s'
[20:38:40]: Turning off FASTLANE_OPTIMIZE_BOOT again.

macOS

[13:40:31]: ---------------------------
[13:40:31]: --- Step: shell command ---
[13:40:31]: ---------------------------
[13:40:32]: 🏎️ 'Total   1.1720s'
[13:40:32]: ---------------------------
[13:40:32]: --- Step: shell command ---
[13:40:32]: ---------------------------
[13:40:34]: 🏎️ 'Total   1.0534s'
[13:40:34]: ---------------------------
[13:40:34]: --- Step: shell command ---
[13:40:34]: ---------------------------
[13:40:35]: 🏎️ 'Total   1.0748s'
[13:40:35]: Turning on FASTLANE_OPTIMIZE_BOOT:
[13:40:35]: ---------------------------
[13:40:35]: --- Step: shell command ---
[13:40:35]: ---------------------------
[13:40:37]: 🏎️ 'Total   1.6660s'
[13:40:37]: ---------------------------
[13:40:37]: --- Step: shell command ---
[13:40:37]: ---------------------------
[13:40:37]: 🏎️ 'Total   0.5803s'
[13:40:38]: ---------------------------
[13:40:38]: --- Step: shell command ---
[13:40:38]: ---------------------------
[13:40:38]: 🏎️ 'Total   0.5173s'
[13:40:38]: Turning off FASTLANE_OPTIMIZE_BOOT again.

Windows

[20:39:46]: ---------------------------
[20:39:46]: --- Step: shell command ---
[20:39:46]: ---------------------------
[20:39:49]: 🏎️ 'Total   2.3745s'
[20:39:49]: ---------------------------
[20:39:49]: --- Step: shell command ---
[20:39:49]: ---------------------------
[20:39:51]: 🏎️ 'Total   2.4303s'
[20:39:51]: ---------------------------
[20:39:51]: --- Step: shell command ---
[20:39:51]: ---------------------------
[20:39:54]: 🏎️ 'Total   2.2891s'
[20:39:54]: Turning on FASTLANE_OPTIMIZE_BOOT:
[20:39:54]: ---------------------------
[20:39:54]: --- Step: shell command ---
[20:39:54]: ---------------------------
[20:39:58]: 🏎️ 'Total   3.8974s'
[20:39:58]: ---------------------------
[20:39:58]: --- Step: shell command ---
[20:39:58]: ---------------------------
[20:40:01]: 🏎️ 'Total   2.2022s'
[20:40:01]: ---------------------------
[20:40:01]: --- Step: shell command ---
[20:40:01]: ---------------------------
[20:40:03]: 🏎️ 'Total   2.0910s'
[20:40:03]: Turning off FASTLANE_OPTIMIZE_BOOT again.

@milch
Copy link
Collaborator Author

milch commented Oct 2, 2018

@janpio Thanks for your testing!

Most of the replacements from require to require_relative had already been done in a previous PR, so this just completes it for fastlane itself and then automates the process for the future.

One big advantage of this plus the previous work that included the missing require checker should be that you'll be able to require any individual file from fastlane anywhere else and have it work. That'll enable us to go through and remove things like require 'spaceship' and replace it with e.g. require 'spaceship/provisioning_profile' which is where I'd expect big speed improvements to come from

@joshdholtz
Copy link
Member

@milch Heyyy 👋 Are you still actively working on this one and wanting this merged in? 🙃

@janpio
Copy link
Member

janpio commented Aug 19, 2019

@mollyIV Are you maybe interested in having another look at the suggested rubocop from this PR and see if this makes sense in the current context and should be re-tried and then merged? Thanks.

@janpio
Copy link
Member

janpio commented Jun 10, 2020

I feat the failing tests and conflicts are the bigger hurdle here :p

@googlebot I consent.

@googlebot
Copy link

CLAs look good, thanks!

ℹ️ Googlers: Go here for more info.

@rogerluan
Copy link
Member

Is there still intent to get this moved forward @milch ? At this point sure if it's worth the effort of solving the merge conflicts, or if it'd be better to start over 😅

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

Successfully merging this pull request may close these issues.

None yet

8 participants