-
Notifications
You must be signed in to change notification settings - Fork 1.9k
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
[close #1072] Don't install bundler twice on CI #1073
Merged
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
schneems
force-pushed
the
schneems/double-bundle-binary-ci-bug
branch
from
September 29, 2020 21:25
644a482
to
c72b18d
Compare
schneems
force-pushed
the
schneems/double-bundle-binary-ci-bug
branch
from
September 30, 2020 20:14
c72b18d
to
cdd9c36
Compare
Shelling out is comparatively expensive and this gets called several times. It's already memoized at the instance level (which calls the class). This commit memoizes the class call as well.
When debugging its common to do something like: ```ruby puts variable ``` And unless it's a string this method will trigger an exception: ``` NoMethodError (undefined method `each_line' for []:Array) ``` This commit ensures all variables passed in to puts will be strings before calling `each_line` on them.
When we compile an app for CI, if the user does not specify a test command than the bin/support/ruby_test gets called. This file automatically detects the test framework the user is likely using and calls the appropriate command (such as `bin/rspec`). The problem is that the logic in this file relies on determining what gems the customer's application has. To detect gems we need access to the bundler's source code because we use bundler internals for parsing the Gemfile.lock. A copy of bundler is already installed on the system from the prior `bin/test-compile` call, but...it's located on disk at the "slug_vendor_base" which is based off of the customer's ruby version, for example: ``` vendor/bundle/ruby/2.6.0 ``` So in order to detect gems, we need access to bundler, to access bundler we need to put the customer's ruby binary on the PATH. This was previously being done here: ``` # The `ruby_test-compile` program installs a version of Ruby for the # user's application. It needs the propper `PATH`, where ever Ruby is installed # we always add a symlink to the `bin/ruby` file so that is always valid. # We calculate the gem path the same way we do when compiling. LanguagePack::ShellHelpers.user_env_hash["PATH"] = "#{build_dir}/bin:#{bundler.bundler_path}/bin:#{ENV["PATH"]}" ``` I moved adding the users `app/bin` to the PATH up to the top of the script. Now that requirement is set, I worked on getting rid of the double bundler installation. In this line: ``` bundler.install ``` It will check to see if a copy of bundler is already installed, the problem is: due to the circular dependency on using bundler to find a customer's Ruby version and then using Ruby to determine where in the customer's app bundler will live, this defaults to a tmp directory. In regular `bin/compile` this is great, we then later move the bundler to the right spot. In this case, we can just tell it the location of where we previously installed bundler in the app: ```ruby bundler = LanguagePack::Helpers::BundlerWrapper.new( gemfile_path: "#{build_dir}/Gemfile", bundler_path: LanguagePack::Ruby.slug_vendor_base # This was previously installed by bin/support/ruby_test-compile ) ``` > In the `slug_vendor_base` With this change bundler checks that directory, sees its non-empty and then skips downloading bundler for a second time. ## Retro This logic is very difficult to reason about. We could make it simpler by removing the requirement that the customer's Ruby version is used to determine the directory of where bundler lives. I don't know what tight coupling might exist between this file location and other actions. For instance when we bundle install we specify a path of `vendor/bundle` and the path we install bundler into is in `vendor/bundle/ruby/<ruby-version>` For a first attempt I would put it in a completely separate directory. Other buildpacks use `.heroku` we could put it in `.heroku/ruby/bundler` and then we wouldn't need to calculate the location. If that raises coupling issues with other parts of this project (likely) then I could iterate again.
``` ENV HATCHET_BUILDPACK_BASE is not set. It currently defaults to the ruby buildpack. In the future this env var will be required ```
schneems
force-pushed
the
schneems/double-bundle-binary-ci-bug
branch
from
September 30, 2020 20:23
cdd9c36
to
609ff76
Compare
danielleadams
approved these changes
Oct 1, 2020
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
When we compile an app for CI, if the user does not specify a test command than the bin/support/ruby_test gets called. This file automatically detects the test framework the user is likely using and calls the appropriate command (such as
bin/rspec
).The problem is that the logic in this file relies on determining what gems the customer's application has. To detect gems we need access to the bundler's source code because we use bundler internals for parsing the Gemfile.lock.
A copy of bundler is already installed on the system from the prior
bin/test-compile
call, but...it's located on disk at the "slug_vendor_base" which is based off of the customer's ruby version, for example:So in order to detect gems, we need access to bundler, to access bundler we need to put the customer's ruby binary on the PATH.
This was previously being done here:
I moved adding the users
app/bin
to the PATH up to the top of the script.Now that requirement is set, I worked on getting rid of the double bundler installation. In this line:
It will check to see if a copy of bundler is already installed, the problem is: due to the circular dependency on using bundler to find a customer's Ruby version and then using Ruby to determine where in the customer's app bundler will live, this defaults to a tmp directory. In regular
bin/compile
this is great, we then later move the bundler to the right spot.In this case, we can just tell it the location of where we previously installed bundler in the app:
With this change bundler checks that directory, sees its non-empty and then skips downloading bundler for a second time.
Retro
This logic is very difficult to reason about. We could make it simpler by removing the requirement that the customer's Ruby version is used to determine the directory of where bundler lives.
I don't know what tight coupling might exist between this file location and other actions. For instance when we bundle install we specify a path of
vendor/bundle
and the path we install bundler into is invendor/bundle/ruby/<ruby-version>
For a first attempt I would put it in a completely separate directory. Other buildpacks use
.heroku
we could put it in.heroku/ruby/bundler
and then we wouldn't need to calculate the location.If that raises coupling issues with other parts of this project (likely) then I could iterate again.