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.

Sign up for GitHub

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

Jump to bottom

Bug when prepending to PATH twice on the same layer #899

Open
schneems opened this issue Jan 10, 2025 · 1 comment
Open

Bug when prepending to PATH twice on the same layer #899

schneems opened this issue Jan 10, 2025 · 1 comment

Comments

@schneems
Copy link
Contributor

schneems commented Jan 10, 2025 •
edited
Loading

Expected

If I prepend PATH with b and then prepend it with a that the outcome will be PATH=b:a:$PATH.

Actual

Only the last modification is preserved, it ends up being PATH=a:$PATH.

Repro

$ cargo new env_delimiter_apply
$ cd env_delimiter_apply
$ cargo add libcnb
$ cargo add pretty_assertions

Stick this code in your main.rs and run tests:

$ cargo test
    Finished `test` profile [unoptimized + debuginfo] target(s) in 0.06s
     Running unittests src/main.rs (target/debug/deps/env_delimiter_apply-487764a010d26946)

running 2 tests
test test::test_simple_path ... ok
test test::test_double_path ... FAILED

failures:

---- test::test_double_path stdout ----
thread 'test::test_double_path' panicked at src/main.rs:46:9:
assertion failed: `(left == right)`

Diff < left / right > :
<PATH=b:a
>PATH=a
@colincasey
Copy link
Contributor

I've been hit by this as well (see #790). My workaround is here.

schneems added a commit to heroku/buildpacks-ruby that referenced this issue Jan 10, 2025
@schneems
Move user binstubs to its own path
6f97cb5 
At runtime, the alphabetical order of the layer name determines the order it's 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 added a commit to heroku/buildpacks-ruby that referenced this issue Jan 10, 2025
@schneems
Move user binstubs to its own path
b17681c 
At runtime, the alphabetical order of the layer name determines the order it's 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 added a commit to heroku/buildpacks-ruby that referenced this issue Jan 10, 2025
@schneems
Move user binstubs to its own layer
c91cee5 
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 added a commit to heroku/buildpacks-ruby that referenced this issue Jan 13, 2025
@schneems
Fix user binstubs not on PATH and fix PATH order (#383)
4452d3a 
* Introduce struct to store rename information

* Add helper to check if a layer exists or not

* Add ability to rename a layer

* Add tests

* Rename `ruby` to `binruby`

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.

* Changelog

* Assert order of ruby/rake

Using `-a` also shows that there's more than one value present.

* Add failing test for `/workspace/bin` being on the path

* Add pretty assertions for nicer diff output

* Expected before actual

* Put user provided binstub dir in the front of the path

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

* Changelog

* Clippy lints

* Integration test for build time PATH

* Clarify that `bundle exec` changes the PATH order

* Fix grammar

* Remove internal `bundle exec` calls

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`.

* Make integration failures prettier

* Lint import order

* Move user binstubs to its own layer

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.

* Extract layer env modification from method

* Update commons changelog date
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@colincasey @schneems