Closed jdost closed 5 years ago
@jerrymarino Thank you so much for looking through those other PRs! When you have some time, it'd be great to get some eyes on this one as well 🙏
Thanks so much for sending over the PR @jdost đź‘Ť . Really like the idea of better handling the extraction bit and this is something we can build upon for git archives. My main suggestion is to remove this from a user level option. I can't see how we'd provide a custom handler.
Currently, if the user needs to provide sources via custom unarchive/extraction logic, it can be achieved by:
install_script
When that PR was written, we didn't support the workspace.bzl
macro - think this is not working there.
Hey @jerrymarino thanks for taking a look over this PR! The motivation for this was to handle URLs like https://codeload.github.com/discordapp/react-native/tar.gz/efef5b756162a72aabff3143591c4ff3edafd3f5, which don't end in a usable extension. We're currently auto-generating a lot of our new_pod_repository
rules from Podfile.lock
and yarn.lock
files, and this in particular comes straight out of yarn.lock
. Auto-generating an install_script
is certainly do-able, but a bit of a pain if/when we need to merge it with the hand-written one we have.
Somewhat separately, I'm not sure how you view install_script
, but it feels to me like a stop-gap and/or pseudo todo list for features to implement natively in PodToBUILD/new_pod_repository
. It seems like a reasonable end goal could be to remove all/most needs for install_script
. But definitely let me know if that is/isn't how you see it! 🙂
@FuegoFro - Install script is sort of the escape hatch and I would say there are definitely cases where its used for things that PodToBUILD could do natively. I'm not sure that it will be removed every since the flexibility is useful for the end user if they have use cases we don't currently have.
Have we considered inferring the mime-type of the file downloaded? We might be able to fallback to this if the file is fetched off the network https://developer.mozilla.org/en-US/docs/Web/HTTP/Basics_of_HTTP/MIME_types/Complete_list_of_MIME_types
I think this is a good PR and would like to merge once we can remove the user specified handler, if possible.
Potential additional ways to find archive type I thought of are, introspecting downloaded archive via the file command or similar.
Otherwise, it should be possible to download and unpack in install script via a custom script. @rahul_malik don’t know if mime types are comprehensive enough..
Would love to see how you’re generating this from the lockfiles - a PR for that is definitely welcome!
BTW: The install script is required to implement prepare_command and non hermetic things that some pods require. It’s also used to resolve incompatibilities in Bazel, patch downloads, and run custom commands. By default, we could run prepare_command to implement the podspec behavior - but it may have security implications.
I’ll be mostly offline for the next couple weeks, and can help after I’m back.
This expands the logic around archive extraction to provide larger explicit filetype support and provides an override parameter to be able to explicitly define the filetype to use and bypass the inference logic.
Changes:
.tar
and.tgz
alongside the existing.tar.gz
checkextraction_handler
and only acceptszip
,tar
, anddefault
(it defaults todefault
which maintains the pre-existing behavior)RepoTool
call as the-handler
flag for thefetch
actiondefault
it will fallback to using the suffix based inference to determine the command.