Set $HOME to $CNB_APP_DIR when running .profile.d/ scripts at launch

[kamaln7]

Some buildpacks (such as heroku-buildpack-php) use profile.d scripts to set up the runtime environment. The expectation is that the app source code is located at $HOME.

Usually this can be addressed by building the stack run image in a way that sets HOME to the same path as CNB_APP_DIR, but this won't work if CNB_APP_DIR is determined during build time.

This configures HOME using env.launch on the profile.d cnb-shim layer. There doesn't seem to be a way for exporter to set custom env vars on the OCI image itself.

Edit: After review this PR now uses approach (2) below instead.

Other approaches:

1. Add a profile.d script that sets HOME to $(pwd) instead of relying on the fact that CNB_APP_DIR is consistent at build and run time, but that would be more complicated because we would need to ensure that it is run before other profile.d scripts.
2. Modify each profile.d script from the shimmed buildpack and add an export HOME=$(PWD) line at the top of the file.

[hone]

@edmorley may have more thoughts here as well. See issue buildpacks/spec#186.

[kamaln7]

This suggestion around runtime env vars was posted a few days ago: buildpacks/community#61. It might be helpful in this context by removing this responsibility from cnb-shim.

[edmorley]

Recently I've been exploring changing the build directory used for standard (non-CI) Heroku builds, from a path like /tmp/build_<hash> to /app to prevent a whole host of relocatability issues (and improve things like caching for parts of the ecosystem that cache based on absolute path; think webpack plugins or bootsnap for Ruby apps).

As part of that work, it's become apparent that we should absolutely not have $HOME be set to the same value as the build directory at runtime, since home is used for ephemeral storage, and very hard to prevent that, since depending on the tools being used the location isn't configurable (think .ssh directory set up for later buildpack).

As such, it's likely I'll be needing to change the build time value of HOME from /app to eg /tmp/home or similar as part of this work.

It would cause a significant amount of breakage to also try and change the runtime value for HOME, so that will have to stay at /app for now. However IMO it's not ideal that this means that the value for HOME will now change between build time and runtime (it's meaning some other buildpacks now need fixing) - so if we were starting from scratch, I would advocate for not allowing HOME to be set to /app at runtime either. (And that's the direction buildpacks/spec#186 is leaning in.)

As such I wonder if cnb-shim should instead try and keep the impacts of setting HOME to /app to just that shimmed buildpack. ie: Set HOME to /app, run the shimmed buildpack's profile.d scripts (so the PATH etc env vars within are resolved to the correct values), then set HOME back to a non-/app value? This would actually be similar to the approach used by one of the subdir buildpacks that plays around with HOME:
https://github.com/SectorLabs/heroku-buildpack-subdir/blob/dbdfeaee66c7c3209f579e4c4fa621ef81cadfdf/bin/compile#L103-L111

[hone]

@kamaln7 Hi, sorry for the delay here. I'm still catching up from last week too (which was already delayed). Given @edmorley's response. What do you think about option 2 you listed?

[kamaln7]

Yeah, I think that sounds like a good option given the additional context. What the subdir buildpack does should work here too. And since we're modifying the profile scripts we might as well use $(pwd) so that we don't hardcode the app dir path at build time.

[hone]

Awesome. Thanks again for putting this together.

[kamaln7]

@hone I've made some changes to be more in line with the suggested approach. Let me know what you think! I haven't yet tested them with an actual buildpack but I did add a test to validate overall functionality.

Also, not sure why CI is failing. It's behind a login. nvm, I just had to log in with any account

[edmorley]

Thank you for the PR and sorry for the delay in coming back to this!

As part of adding the upcoming Heroku-24 stack, we audited the current Heroku-20/22 CNB base image variants to try and bring them closer in line with the spec and/or what other providers are using for their base images. One of the changes made as a result, was to drop the somewhat hacky overriding of HOME to a fixed value of /app.

As such, longer term I'd also like to backport this HOME hack removal to our Heroku-20/22 CNB base image variants, since our non-shimmed builders (heroku/builder:20 and heroku/builder:22 use the same base images, and are therefore currently exposed to this hack, and will end up being inconsistent with heroku/builder:24).

However, before we could think about making that change, we'd need cnb-shim to set HOME to a value that still works for the shimmed buildpacks, which this PR does. (IMO this PR is what should have landed in the first place rather than setting HOME=/app in the base images themselves back in the early proof of concept days.)

As such, even though cnb-shim is EOL (and doesn't work with our modern builders, so we're not really investing in the tool for now), I'm going to merge/release this change now - before we stop building our legacy builders (at which point they won't pick up any cnb-shim changes) to unblock that potential base image cleanup in the future.

I've tested this PR using the legacy builder images smoke tests - by editing the builder.toml configs to use the Review App URL instead of the main cnb-shim app's URL - and all tests pass:
https://github.com/heroku/cnb-builder-images/actions/runs/8753865189

Plus ran another test run emulating what would happen when we eventually stop overriding HOME in the run images (this CI run uses a custom run image that restores HOME to its correct value) - and all tests pass here too:
https://github.com/heroku/cnb-builder-images/actions/runs/8754431732

As such, I'm happy to merge/release this -- I've rebased on main to resolve conflicts, added ALLOW_EOL_SHIMMED_BUILDER=1 to the test so it passes now that #90 has landed, and made a couple of style/quoting tweaks, but otherwise everything else is the same and looks great - thank you :-)