Use symlinks instead of moving build directory #278
Conversation
Malax
force-pushed
the
malax/use-symlinks-for-build-dir
branch
4 times, most recently
from
b2149b5 to
4b9530a
Compare
Malax
force-pushed
the
malax/use-symlinks-for-build-dir
branch
from
4b9530a to
7ccd451
Compare
Malax
force-pushed
the
malax/use-symlinks-for-build-dir
branch
from
7ccd451 to
825bc8b
Compare
sbt relies on stable paths for its incremental compilation cache. The buildpack previously moved the entire build directory to a temporary location to provide a stable path, then moved it back at the end. This change replaces the expensive move operations with a symlink, providing the same stable path for sbt while keeping files in their original location. This eliminates the need to rewrite PATH and GEM_PATH variables to account for the directory move.
Malax
force-pushed
the
malax/use-symlinks-for-build-dir
branch
from
825bc8b to
4359d7c
Compare
| metrics::init "${CACHE_DIR}" "scala" | ||
| metrics::setup | ||
|
|
||
| if is_app_dir "${APP_BUILD_DIR}"; then |
There was a problem hiding this comment.
(for another PR) The function is called is_app_dir but it actually seems to checks for whether the directory is not the /app dir?
| if is_app_dir "${APP_BUILD_DIR}"; then | ||
| BUILD_DIR="/tmp/scala_buildpack_build_dir" | ||
| mv "${APP_BUILD_DIR}" "${BUILD_DIR}" | ||
| ln -s "${APP_BUILD_DIR}" "${BUILD_DIR}" |
There was a problem hiding this comment.
(optional) I find these variable names confusing, given in most other buildpacks the BUILD_DIR really does mean the actual build dir. For example the rm "${BUILD_DIR}" later looks really bad until you know it's only the symlink!
I wonder if the symlink path should be named something else?
| EOF | ||
|
|
||
| # Move compiled app back to where Heroku expects it | ||
| # Remove symlink if we created one |
There was a problem hiding this comment.
(background only) For Python and PHP we don't remove the symlinks after, in case later buildpacks happen to call those paths. I imagine that's less likely to happen for Scala though.
| metrics::init "${CACHE_DIR}" "scala" | ||
| metrics::setup | ||
|
|
||
| if is_app_dir "${APP_BUILD_DIR}"; then |
There was a problem hiding this comment.
I don't see a Heroku CI Hatchet test in this repo - and Heroku CI is one of the cases where the /app directory gets used instead of /tmp/build_* - would it be worth adding a Heroku CI test?
(The other case when the app dir varies is when the build-in-app-dir labs is enabled - I don't bother having a hatchet test for that for Python, but do make sure to test that labs manually when making any /app vs /tmp path-specific changes)
2 participants
The buildpack moves the build directory to
/tmp/scala_buildpack_build_dirto provide a stable path for sbt's incremental compilation cache, which invalidates when paths change between builds.This changes the approach from moving the directory (and moving it back at the end) to creating a symlink at the stable path pointing to the original location. This eliminates the expensive move operations while maintaining the same stable path for sbt.
sbt invocation will get overhauled extensively in a separate PR, this PR only focusses on the
cptoln -schange.Note that the tests that validate caching/incremental compiles added in #284 and #287 still pass after minimal modification because of the changed directory.
The same is true for the test added in #269 for the now removed
PATH/GEM_PATHrewrites. The test continues to be part of this repository unchanged for now but will the comment wording of the comment will change in a integration test overhaul PR soon.GUS-W-20010421