[MRG] Allow absolute paths in build_script_files #681
Conversation
repo2docker/buildpacks/base.py
Outdated
| src_parts = src_path.split('/') | ||
| src_path = os.path.join(os.path.dirname(__file__), *src_parts) | ||
| BLOCKSIZE = 65536 | ||
| hash_md5 = hashlib.md5() |
There was a problem hiding this comment.
I think we can get away with hashing the absolute path instead of having to hash the contents of the file. What do you think?
There was a problem hiding this comment.
Also let's use sha256, it should be fast enough - especially if we're just hashing the filename.
There was a problem hiding this comment.
I assumed that build scripts are small enough so that it won't matter, but I'm happy to oblige.
There was a problem hiding this comment.
Changed to sha256 and computing slug using filepath only.
repo2docker/buildpacks/base.py
Outdated
| while len(buf) > 0: | ||
| hash_md5.update(buf) | ||
| buf = fh.read(BLOCKSIZE) | ||
| return hash_md5.hexdigest(), src_path |
There was a problem hiding this comment.
We should probably include at least some part of the filename in the resulting filepath we return, to make debugging easier. How about something like: assemble_files/<escaped-file-path-truncated>-<6-chars-of-hash> or something like that?
See https://github.com/jupyterhub/binderhub/blob/6b2908d7aaf4a7ec62beed0019de54db06494214/binderhub/builder.py#L127 where we do something like that, keeping in mind the 255 char linux filepath limit.
There was a problem hiding this comment.
Fixed in 5447508
repo2docker/buildpacks/base.py
Outdated
| @@ -481,13 +482,38 @@ def render(self): | |||
| labels=self.get_labels(), | |||
| build_script_directives=build_script_directives, | |||
| assemble_script_directives=assemble_script_directives, | |||
| build_script_files=self.get_build_script_files(), | |||
| build_script_files={ | |||
There was a problem hiding this comment.
Should probably move this to a temporary variable for clarity, and add a comment about what it's doing.
Xarthisius
force-pushed
the
abspath_in_scripts
branch
from
f4cf551 to
9f1fa93
Compare
|
@yuvipanda I believe I addressed all your inline comments and added tests. |
repo2docker/buildpacks/base.py
Outdated
| return escapism.escape(s, safe=safe_chars, escape_char='-') | ||
|
|
||
| src_path_slug = escape(src_path) | ||
| filename = 'assemble_files/{name}-{hash}' |
There was a problem hiding this comment.
| filename = 'assemble_files/{name}-{hash}' | |
| filename = 'build_script_files/{name}-{hash}' |
|
@Xarthisius one minor change I proposed (making up for an original mistake I made in my review!) otherwise LGTM. The failing test is due to #684 I believe. |
|
@Xarthisius I've now merged #684. Can you rebase to see if all the tests pass? Thank you |
Xarthisius
force-pushed
the
abspath_in_scripts
branch
from
9f1fa93 to
a863949
Compare
Xarthisius
force-pushed
the
abspath_in_scripts
branch
from
a863949 to
a7a9a76
Compare
Xarthisius
force-pushed
the
abspath_in_scripts
branch
from
a7a9a76 to
24234d9
Compare
Xarthisius
changed the title
minrk
changed the title
[MRG] Allow absolute paths in build_script_files. Fixes jupyterhub#673
Fixes #673
This PR allows to provide an absolute path for source files in
get_build_script_files. As a result, plugins can inject files outside of r2d's repository. In case, source file path is relative, the old behavior is preserved, i.e. path is assumed to be relative to the directory whererepo2docker/buildpacks/base.pyis.@yuvipanda PTAL!