-
Notifications
You must be signed in to change notification settings - Fork 1.8k
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.
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
Evaluate symlinks when comparing $BUILD_DIR against /app #992
Conversation
Hmmm I feel like Travis CI is failing because of an unrelated issue?
file.write <<-EOF
machine git.heroku.com
login #{ENV.fetch('HEROKU_API_USER')}
password #{ENV.fetch('HEROKU_API_KEY')}
EOF |
@kamaln7 Hi! Thank you for the PR. The reason Travis is failing is since those credentials are only set for non-fork PRs. This is on my list to review/trigger a full CI run with creds next week - in the meantime could you rebase on master and fix the merge conflict in |
Thank you, @edmorley! That explains it :) |
5531392
to
5739e50
Compare
5739e50
to
5e91502
Compare
Hmm so the unit tests fail on Cedar-14 (eg https://travis-ci.com/github/heroku/heroku-buildpack-python/jobs/369755972), since for Ubuntu 14.04 coreutils doesn't include Our options are to either update https://github.com/heroku/stack-images to install the Researching the difference between the two I found: ...which muddies the water further by suggesting that |
Ah thanks @edmorley I admit that I had only tested this on 18.04. I agree that if we should avoid modifying the stack images if we can. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That change sounds good - thank you! Marking this as changes required for now to remove it from my pending reviews list :-)
I've opened heroku/base-images/pull/176 to add |
Hi @edmorley very sorry for the delay here. I've just pushed a commit updating the code to use |
2e87491
to
98e81d1
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kamaln7 Sorry for the delay here. In the meantime, the Cedar-14 stack has been sunset, meaning all buildable stacks now support realpath
. As such, I've reverted the commit to switch to readlink
. I've rebased the PR, updated another BUILD_DIR
comparison to use realpath
too, and updated the changelog entry for the new style. I've triggered CI (now using Circle) which is passing.
Meant to say - in the future I hope to remove the |
Hey @edmorley, thanks for the context & the additional fix! |
This updates the
$BUILD_DIR
check to evaluate symlinks before comparing to/app
.In our build environment we have
/app
symlinked to/workspace
to accommodate other buildpacks.$BUILD_DIR
is set to/workspace
. That causes this buildpack to fail to build because the paths aren't equal so the.heroku/*
symlinks are created and in a later step the buildpack tries to install python over the symlink.