Bug in PATHs

[samwillis]

Hi,

Firstly, thanks for this! From trying out a few vips build packs this seems to be the best and most current one!

I have found a bug in the way the PATHs are updated, and it seems the bug has been there a while. In fact its apparent in every build pack that uses the same bash vendor() function and there are a good few of them (at least 16 other build packs, see: https://github.com/search?l=&p=2&q=%22function+vendor%28%29+%7B%22+buildpack+language%3AShell&type=Code)

Effectively PATHs are constructed twice in the bin/compile script which results in the PATHs of previous build packs being backed into the environment setup script of the current build pack. This results in them appearing twice and out of order. I discovered it when using this with the python build pack and having /usr/local/bin appear before /app/.heroku/python/bin in the PATH with the wrong python being used.

My PATH was generated as:

/app/bin:/app/vendor/vips/bin:/app/.apt/usr/bin:/usr/local/bin:/usr/bin:/bin:/tmp/codon/vendor/bin:/app/.heroku/python/bin:/app/.heroku/node/bin:/app/.heroku/yarn/bin:/app/.apt/usr/bin:/usr/local/bin:/usr/bin:/bin:/app/bin:/app/node_modules/.bin

Note the repetition, out of order and /tmp/codon/vendor/bin which should not be there in the deployed slug, its an artefact from the build process (Herokus build playroom is called codon)

After fixing it we get this which is correct:

/app/bin:/app/vendor/vips/bin:/app/.heroku/python/bin:/app/.heroku/node/bin:/app/.heroku/yarn/bin:/app/.apt/usr/bin:/usr/local/bin:/usr/bin:/bin:/app/bin:/app/node_modules/.bin

I have fixed it in my fork here: samwillis@7e47b1d

[samwillis]

It looks to me like it dates back to this build pack: https://github.com/mojodna/heroku-buildpack-pgsql

and affects most of @mojodna build packs.

[brandoncc]

Thanks for the kind words and the opened issue @samwillis! I wonder if this was the reason I had an issue with my path and had to prepend /app/bin to it in the compile script. I will definitely be trying your fork and if it works for my app (I assume it does), I'd like to merge it into my repo.

[mojodna]

@samwillis thanks for the heads-up! Sorry for the confusion that this has caused. My buildpacks are mostly out-of-date at this point, but I'll see if it makes sense to update some of them to reflect your fix.

[samwillis]

@mojodna no worries, in our fast moving world these things get outdated before we can blink.

If you do decided to change any my fix only works when calling vendor() once, you will want to change it to build up any additions from multiple vendor() calls before adding them at the end for a couple of your buildpacks.

As an aside, its also a small world, I'm still using your tessera project for something from a few years ago, also a great project!

[mojodna]

🥂🍻

[jonian]

I'm getting an error when using the buildpacks config below with a rails application:

https://github.com/heroku/heroku-buildpack-apt
https://github.com/brandoncc/heroku-buildpack-vips
https://github.com/gaffneyc/heroku-buildpack-jemalloc
https://github.com/heroku/heroku-buildpack-ruby

!     Could not detect rake tasks
remote: : Could not detect rake tasks (LanguagePack::Helpers::RakeRunner::CannotLoadRakefileError)

If I put vips buildpack after the ruby buildpack it works without problems. Looks like it is due to this bug.

Paths without vips buildpack:

/app/bin
/app/vendor/bundle/bin
/app/vendor/bundle/ruby/3.1.0/bin
/app/.apt/usr/bin
/usr/local/sbin
/usr/local/bin
/usr/sbin
/usr/bin
/sbin
/bin
/app/vendor/jemalloc/bin

Paths with vips buildpack:

/app/bin
/app/vendor/vips/bin
/app/.apt/usr/bin
/app/bin
/app/vendor/bundle/bin
/app/vendor/bundle/ruby/3.1.0/bin
/app/vendor/ruby-3.1.0/bin
/tmp/tmp.pL3bsbSBgW/bin/
/usr/local/sbin
/usr/local/bin
/usr/sbin
/usr/bin
/sbin
/bin
/app/bin
/usr/local/sbin
/usr/local/bin
/usr/sbin
/usr/bin
/sbin
/bin
/app/bin
/app/vendor/bundle/bin
/app/vendor/bundle/ruby/3.1.0/bin
/app/.apt/usr/bin
/usr/local/sbin
/usr/local/bin
/usr/sbin
/usr/bin
/sbin
/bin
/app/vendor/jemalloc/bin

[brandoncc]

Well, it took me four years, but I finally merged this fix. Thank you for opening this issue @samwillis.

To be honest, I was nervous that I wouldn't know how to properly test the changes and I would break the buildpack for everyone. I've been doing quite a bit of work on it the last couple of weeks and I finally got over that fear. Sorry for the wait!