Skip to content
This repository has been archived by the owner on Sep 6, 2022. It is now read-only.

Make Bundler load Project's Gemfile dependencies #703

Merged
merged 4 commits into from
Apr 20, 2018

Conversation

minuscorp
Copy link
Contributor

@minuscorp minuscorp commented Apr 19, 2018

Fixes #674

@minuscorp minuscorp changed the title Make Bundler load Project's Gemfile dependencies [WIP] Make Bundler load Project's Gemfile dependencies Apr 19, 2018
logger.error("The following gems are missing")
not_installed.each { |s| logger.error(" * #{s.name} (#{s.version})") }
end
rescue Bundler::GemfileNotFound => ex
Copy link
Collaborator

Choose a reason for hiding this comment

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

let's use notification_service here too, here's an example:

def handle_exception(ex, console_message: nil, exception_context: {})

Ideally, we send this notification to the admin of the machine so they can potentially file a bug, or something

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Nice! I'll first try to make it work properly but that's a good one!!

Copy link
Collaborator

Choose a reason for hiding this comment

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

Cool, there's already a notification_service property on the super class 👍

@taquitos taquitos mentioned this pull request Apr 19, 2018
7 tasks
Copy link
Member

@KrauseFx KrauseFx left a comment

Choose a reason for hiding this comment

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

Oh wow, great job on finding out how to use this, it looks great. Can we verify that this works, by trying to onboard fastlane/fastlane, where we make use of fastlane plugins?

@minuscorp
Copy link
Contributor Author

minuscorp commented Apr 19, 2018

This is becoming to a dead end. When I success in loading the Gemfile, it fails if there's already the same gem (different version loaded) and I don't see any way to unload the gems of the current process, and bundler seems not to achieve the isolation... I'll try to work on it this evening but is not looking good 😔

@KrauseFx KrauseFx added the status: blocked Waiting for something else before we can proceed label Apr 19, 2018
@minuscorp minuscorp removed the status: blocked Waiting for something else before we can proceed label Apr 19, 2018
@minuscorp minuscorp changed the title [WIP] Make Bundler load Project's Gemfile dependencies Make Bundler load Project's Gemfile dependencies Apr 19, 2018
@minuscorp
Copy link
Contributor Author

So this is the output of a Project with a Gemfile that uses the betterlorem gem in the lane:

# Logfile created on 2018-04-20 00:59:14 +0200 by logger.rb/56438
[00:59:14]: ------------------------------
[00:59:14]: --- Step: default_platform ---
[00:59:14]: ------------------------------
[00:59:14]: gem 'betterlorem' is already installed
[00:59:14]: Driving the lane 'ios test' 🚀
[00:59:14]:
Terra Zabulon, et terra Nephthalim, via maris trans Jordanem, alilæa gentium populus, qui sedebat in.
[00:59:14]: fastlane.tools finished successfully 🎉

So the only way that I accomplished this is doing the following:

  1. Read and store the state of the CI Gemfile and Gemfile.lock
  2. Inject all dependencies (reject fastlane, that we use the CI's version). This make changes in both files in step 1.
  3. Install and require the new gems detected from the building repo.
  4. Write the original contents to the Gemfile and Gemfile.lock before finishing (injecting modifies both files).

This works. Is not the perfect solution? I don't think so, but I can't make with another solution.

@taquitos
Copy link
Collaborator

@minuscorp does this merge the dependencies of CI and whatever project is being built?

@minuscorp
Copy link
Contributor Author

This temporally adds dependencies of other projects to the CI (while building) then, CI's original dependencies are restored and installed again.

@taquitos
Copy link
Collaborator

Ok, let's go with it for now, but ultimately I think we need to actually fork a new process to avoid this approach.
Realistically speaking, we're a mobile ci, so we're mainly seeing this issue because we're running CI for CI.
During normal usage I don't think people will run into this issue.

Copy link
Collaborator

@taquitos taquitos left a comment

Choose a reason for hiding this comment

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

🕶:goat:

@minuscorp
Copy link
Contributor Author

minuscorp commented Apr 20, 2018

Yep, I know this is super hacky, but while we're attached to the same thread as the CI for the builds I couldn't find any other way to workaround this. In a future the whole fastlane build runner should(?) be a RecreatableTask with its own scope, where it would be able to grab the CI fastlane dependency, inject it to the build, and install all the other gems (if any).

Maybe we should open a discussion about this, in a follow up PR/issue.

@taquitos
Copy link
Collaborator

💯 agree on that. It should be it's own process so there isn't any polluting of lane contexts like we have now, and it would enable us to have better resiliency to crashes and other issues.
Ideally we'd spawn a new process and inject all the dependencies like you said, then use some form of IPC to monitor the build.

@@ -98,30 +98,50 @@ def run(new_line_block:, completion_block:)
# folder, and all the following code works
# This is needed to load other configuration files, and also find Xcode projects

original_gemfile = File.open(Bundler.default_gemfile, "rb")
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this to:

original_gemfile_contents = File.read(Bundler.default_gemfile)
original_lockfile_contents = File.read(Bundler.default_lockfile)

@@ -153,6 +173,15 @@ def run(new_line_block:, completion_block:)
artifacts_paths = gather_build_artifact_paths(loggers: [verbose_log, info_log])

return
ensure
if gemfile_found
Copy link
Contributor

Choose a reason for hiding this comment

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

Could we simplify this ensure block to:

ensure
  return unless gemfile_found
  original_gemfile_contents = File.read(Bundler.default_gemfile)
  original_lockfile_contents = File.read(Bundler.default_lockfile)
end

Copy link
Contributor Author

Choose a reason for hiding this comment

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

agh, never return in a ensure block 😢

Copy link
Contributor

Choose a reason for hiding this comment

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

Forgot that, good catch.

@minuscorp
Copy link
Contributor Author

Thinking out loud, unidirectional IPC -> IO.pipe, bidirectional IPC -> Socket.pair. This would change a lot of the things as they are designed right now though

@minuscorp minuscorp merged commit d7c923d into fastlane:master Apr 20, 2018
Copy link
Contributor

@armcburney armcburney left a comment

Choose a reason for hiding this comment

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

Thank you so much @minuscorp for fixing the gemfile dependencies issue! I just have two minor nitpicks, but great work!

minuscorp added a commit that referenced this pull request Apr 20, 2018
@minuscorp
Copy link
Contributor Author

Oh man, just squashed :(

@armcburney
Copy link
Contributor

Ahh no worries - we can do it in another PR, or leave it the way it is 🙂

minuscorp added a commit to minuscorp/ci that referenced this pull request Apr 20, 2018
minuscorp added a commit to minuscorp/ci that referenced this pull request Apr 20, 2018
minuscorp added a commit that referenced this pull request Apr 20, 2018
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants