[close #943] Use Bundler env vars for config #1039
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/bundler-env-varrrrrrrrrrrrrrs
branch
5 times, most recently
from
54b85c7
to
4707a99
Compare
schneems
force-pushed
the
schneems/bundler-env-varrrrrrrrrrrrrrs
branch
from
28c4f65
to
ea4d677
Compare
|
(Removed languages-review label since I'm on-call but this seems best reviewed by @hone who is already set as reviewer :-)) |
schneems
force-pushed
the
schneems/bundler-env-varrrrrrrrrrrrrrs
branch
2 times, most recently
from
31234af
to
e24c923
Compare
Starting with Bundler 2.x deprecation warnings started appearing noting that flags will no longer persist config when running `bundle install`:
```
Running: bundle install --without development:test --path vendor/bundle --binstubs vendor/bundle/bin -j4 --deployment
[DEPRECATED] The `--deployment` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set deployment 'true'`, and stop using this flag
[DEPRECATED] The `--path` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set path 'vendor/bundle'`, and stop using this flag
[DEPRECATED] The `--without` flag is deprecated because it relies on being remembered across bundler invocations, which bundler will no longer do in future versions. Instead please use `bundle config set without 'development:test'`, and stop using this flag
[DEPRECATED] The --binstubs option will be removed in favor of `bundle binstubs`
```
This is generally a confusing part of running bundler locally where someone on a team might try running `--bundle_without` once and then be confused at future behavior.
To support persisting values, we can use the bundler env vars that start with `BUNDLE_`. These will be set at:
- Build time
- exported for other buildpacks
- Runtime
In addition, they're being shown in the `bundle install` output so if a developer gets a failure on Heroku they can copy and paste the command to test locally.
schneems
force-pushed
the
schneems/bundler-env-varrrrrrrrrrrrrrs
branch
from
e24c923
to
bd30fff
Compare
hone
reviewed
Aug 13, 2020
lib/language_pack/ruby.rb
Outdated
| set_export_default "BUNDLE_PATH", ENV["BUNDLE_PATH"], layer | ||
| set_export_default "BUNDLE_WITHOUT", ENV["BUNDLE_WITHOUT"], layer | ||
| set_export_default "BUNDLE_BIN", ENV["BUNDLE_BIN"], layer | ||
| set_export_default "BUNDLE_DEPLOYMENT", ENV["BUNDLE_DEPLOYMENT"], layer if ENV["BUNDLE_DEPLOYMENT"] |
There was a problem hiding this comment.
doesn't setup_language_pack_environment always set this to be 1?
There was a problem hiding this comment.
If there's a windows Gemfile.lock then we delete it and remove the deployment mode https://github.com/heroku/heroku-buildpack-ruby/pull/1039/files#diff-3fc66e13e389ff4d15c7ca3ddb86464eR886
There was a problem hiding this comment.
ah yeah, i wonder if it's worth just adding a comment.
There was a problem hiding this comment.
I added a comment. Thinking about it a bit more, it's unclear this behavior is needed for the export and profile.d files i.e. once we've already run bundle install then there's not much benefit in keeping the deployment mode off. The PR as is preserves existing behavior, so let's not muck it up for now. Just keep in mind we could likely make that change if we wanted.
hone
reviewed
Aug 13, 2020
| ENV["BUNDLE_WITHOUT"] = env("BUNDLE_WITHOUT") || bundle_default_without | ||
| ENV["BUNDLE_PATH"] = bundle_path | ||
| ENV["BUNDLE_BIN"] = bundler_binstubs_path | ||
| ENV["BUNDLE_DEPLOYMENT"] = "1" |
There was a problem hiding this comment.
If we display all the env vars when running the command, do we want to make BUNDLE_DEPLOYMENT configurable?
There was a problem hiding this comment.
I don't think there's a reason to deploy without --deployment mode unless they're using a windows Gemfile.lock.
hone
approved these changes
Aug 18, 2020
The code makes it difficult to see what the conditions where this would be empty are. This comment makes that condition more clear.
schneems
added a commit
that referenced
this pull request
Aug 19, 2020
schneems
added a commit
that referenced
this pull request
Sep 3, 2020
* Revert "Revert "[close #943] Use Bundler env vars for config (#1039)" (#1047)" This reverts commit 858adcb. * Fix gem installation location bug with Bundler env vars When the move from bundler flags to bundler env vars was first merged in an issue was reported #1046 , this lead to an investigation and bug report to bundler rubygems/rubygems#3890 which lead to some older issues/prs: - rubygems/bundler#3552 - rubygems/bundler#6628 The issue is that global configuration and local configuration results in different behavior when installing and using some features in bundler, notedly the ability to specify install path. Due to this change, when the switch to bundler environment variables happened, it caused the size of some applications' slugs to increase dramatically, this was because the gems were essentially being installed twice. The issue appears to not be in Bundler 2.1.4 so we're bumping the version for 2.x series. In the 1.x series an environment variable `BUNDLE_GLOBAL_PATH_APPENDS_RUBY_SCOPE=1` can be used to force the desired behavior. # This is the commit message #2: Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com> Co-authored-by: Ed Morley <501702+edmorley@users.noreply.github.com>
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.
Starting with Bundler 2.x deprecation warnings started appearing noting that flags will no longer persist config when running
bundle install:This is generally a confusing part of running bundler locally where someone on a team might try running
--bundle_withoutonce and then be confused at future behavior.To support persisting values, we can use the bundler env vars that start with
BUNDLE_. These will be set at:In addition, they're being shown in the
bundle installoutput so if a developer gets a failure on Heroku they can copy and paste the command to test locally.