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

Fix PATH to vendored Ruby #108

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Fix PATH to vendored Ruby #108

wants to merge 1 commit into from

Conversation

r4victor
Copy link

@r4victor r4victor commented Sep 2, 2022

Fixes #107

edmorley
edmorley requested changes Sep 2, 2022
View reviewed changes
Copy link
Member

@edmorley edmorley left a comment •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi!

Thank you for the PR, however I don't believe this is the correct fix.

After this change, /app will now be hardcoded in the .profile.d/ script, when $HOME is not always /app (for example some buildpacks modify $HOME to facilitate other use-cases).

It also seems slightly broken for individual buildpacks like this one, to have to work around other non-standard project layout buildpacks. It feels the correct fix here is for heroku-buildpack-subdir to update $HOME at runtime to avoid the issues here - rather than dozens of buildpacks to have to add their own workarounds.

@r4victor
Copy link
Author

r4victor commented Sep 2, 2022

I wasn't aware of buildpacks modifying $HOME. But this is not a problem – we can use it instead of hardcoded /app. Will it do?

heroku-buildpack-subdir can't fix this by modifying $HOME since there can be multiple buildpacks but only one $HOME.

The reason why I think the fix belongs here is because of the inconsistency: compile puts the ruby executable in the location controlled by $BUILD_DIR but ignores it when setting $PATH.

Other buildpacks that I'm aware of that modify $PATH work with heroku-buildpack-subdir because they install to /app/.heroku/ and symlink the $BUILD_DIR-based path. For example, see Python buildpack, PHP buildpack. Since nginx buildpack already installs to $BUILD_DIR-based path, I think the best approach here is to set $PATH accordingly.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@edmorley edmorley edmorley requested changes

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

PATH to vendored Ruby is incorrect if buildpack dir differs from the default
2 participants
@r4victor @edmorley