Skip to content

Conversation

@mbrobbel
Copy link
Member

As suggested in #12322 (comment).

@github-actions
Copy link

Co-authored-by: Joris Van den Bossche <[email protected]>
@jorisvandenbossche
Copy link
Member

@kszucs it seems that (most?) other uses of URL in our cmake files also includes a URL_HASH. Should we add that for substrait as well?

@kszucs
Copy link
Member

kszucs commented Feb 23, 2022

I think the purpose of the ARROW_SUBSTRAIT_REPO variable is that the developer can quickly switch to a substrait fork with altered protobuf definitions. Introducing a checksum variable would make that less convenient.

@kszucs kszucs requested a review from bkietz February 23, 2022 10:04
@mbrobbel
Copy link
Member Author

@jvanstraten

@jvanstraten
Copy link
Contributor

Uh oh, looks like there's been some redundant, parallel work going on... See #12457

tl;dr: the whole setup with the ARROW_SUBSTRAIT_REPO option and such was in and of itself not very conformant to the way the rest of Arrow's build works, and the git shenanigans broke a lot more than just failure to build without git when doing inline builds. Above PR converts the entire build for Substrait to the more usual ThirdpartyToolchain.cmake workflow and should supersede this. AFAIK it's ready to merge.

I think the purpose of the ARROW_SUBSTRAIT_REPO variable is that the developer can quickly switch to a substrait fork with altered protobuf definitions. Introducing a checksum variable would make that less convenient.

It wouldn't work without changing code (including CMake stuff) anyway, since Substrait is still too volatile for that. That said, it'd be useful to have a proper way to override dependency versions and download URL patterns using options for development purposes, which ideally would also just disable the hash check.

@mbrobbel mbrobbel closed this Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants