[stacked] Spike: Auto Layer Ordering #384
Closed
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
Note that this places `/layers/heroku_ruby/gems/bin` before `/layers/heroku_ruby/binruby` on the path. Related to, but doesn't entirely fix #380.
Using `-a` also shows that there's more than one value present.
It's common and expected that Rails applications will include a `bin` directory containing "binstubs" of executables their app depends on. For example https://github.com/heroku/ruby-getting-started/tree/5e7ce01610a21cf9e5381daea66f79178e2b3c06/bin. They're largely used to ensure that bundler is invoked/used so that you can run `bin/rails` rather than needing to use `bundle exec rails`. However it's not strictly limited to only that. This change: Adds the `bin` folder in the root of the workspace to the PATH and changes the layer to `venv` so it is loaded after other layers (and takes precedence in the case of a PATH prepend). This fixes the previously committed failing test. Close #380
The Application Contract does not specify that commands such as `rake -P` will be called with `bundle exec` and the classic buildpack does not rely on `bundle exec` internally. This brings the CNB closer to parity with the classic buildpack. In the container environment, the first gems on the PATH should be those installed by the buildpack, negating the strict need to call `bundle exec` as you would on a development machine. Usually prepending a Ruby command with `bundle exec` will have no discernible difference for an application that's bug free. This is evidenced by all tests passing with this change. However someone can commit their own `bin/rake` or `bin/rails` and we should use this over the executable installed via `bundle install`.
schneems
force-pushed
the
schneems/add-user-binstubs
branch
from
6f97cb5 to
b17681c
Compare
schneems
force-pushed
the
schneems/auto-layer-ordering
branch
from
c51ddc6 to
1076e8b
Compare
At runtime, the alphabetical order of the layer name determines the order it is loaded. At build time, the order that the `env` variable is modified determines the order. At both build and runtime we want the bin stubs to come first on the PATH when executing user defined code. This was already working for runtime, but wasn't for build time as the "gems" layer was being prepended to the path after the "venv" layer (because the `venv` layer was being defined first, last definition wins). I originally tried to fix this by defining the PATH inside of the "gems" layer along with the gems path but ran into heroku/libcnb.rs#899. The libcnb.rs project loads the user defined PATH modification last, but I'm unclear if that's spec defined behavior or not https://github.com/buildpacks/spec/blob/main/buildpack.md#layer-paths.
schneems
force-pushed
the
schneems/add-user-binstubs
branch
from
b17681c to
c91cee5
Compare
Prefix layers with sequential numbers starting at `0001_<name>`. To support this it assumes any layers on disk with a number prefix is equivalent i.e. changing the order of layers will not invalidate the cache. This has the benefit that: - Layers can be named something semantic without side effects. - Build and launch layer behavior is guaranteed to be the same (provided `read_env` is called and applied for every layer in main).
schneems
force-pushed
the
schneems/auto-layer-ordering
branch
from
1076e8b to
b6820b6
Compare
|
I think it makes more sense to have a single function/macro that does it instead of trying to re-order things behind the scenes automatically, though we still need the conversion code. |
schneems
force-pushed
the
schneems/auto-layer-ordering
branch
from
40b17aa to
9e17152
Compare
|
Ed pointed out that not all binaries can be relocated and might embed path information in the binary itself. I think ordering needs to be a first-class feature from CNB and opened buildpacks/rfcs#322 to explore what the interface might look like based on my experiences here. |
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.
Prefix layers with sequential numbers starting at
0001_<name>. To support this it assumes any layers on disk with a number prefix is equivalent i.e. changing the order of layers will not invalidate the cache.This has the benefit that:
read_envis called and applied for every layer in main).