Skip to content

Add GCS dependencies to resolve_dependency and refactor cmake modules#9072

Closed
tigrux wants to merge 1 commit intofacebookincubator:mainfrom
tigrux:gcs_bigquery_deps
Closed

Add GCS dependencies to resolve_dependency and refactor cmake modules#9072
tigrux wants to merge 1 commit intofacebookincubator:mainfrom
tigrux:gcs_bigquery_deps

Conversation

@tigrux
Copy link
Copy Markdown
Contributor

@tigrux tigrux commented Mar 13, 2024

The goal of this change is to refactor and simplify the existing cmake modules.

@tigrux tigrux marked this pull request as draft March 13, 2024 20:44
@facebook-github-bot facebook-github-bot added the CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. label Mar 13, 2024
@netlify
Copy link
Copy Markdown

netlify bot commented Mar 13, 2024

Deploy Preview for meta-velox canceled.

Name Link
🔨 Latest commit 9f99aec
🔍 Latest deploy log https://app.netlify.com/sites/meta-velox/deploys/665e4cefc5f3a400085bd381

Copy link
Copy Markdown
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see that it works with the current protobuf version, that's nice.

I think updating the cmake version is great OVERRIDE_FINDE_PACKAGE is really useful. I am not sure what we gain from EXCLUDE_FROM_ALL though?

Any time resolve_dependencies get's called for any of these they will be needed as a dependency for other target's so they will get build?

@tigrux tigrux marked this pull request as ready for review May 16, 2024 17:06
@tigrux tigrux changed the title WIP: Enable dependencies of Google BigQuery Enable dependencies of Google BigQuery May 16, 2024
@tigrux tigrux requested review from assignUser, bdice and kgpai May 16, 2024 20:13
@assignUser
Copy link
Copy Markdown
Collaborator

Once #9861 is merged you can remove the install bits in the workflows and rebase on it (you'll have to wait a bit after the merge for the rebuild of the containers). I'll review after that but looks good so far, thanks for the contribution! :)

@tigrux tigrux changed the title Enable dependencies of Google BigQuery Refactor cmake modules May 22, 2024
@tigrux
Copy link
Copy Markdown
Contributor Author

tigrux commented May 22, 2024

Hello @majetideepak, @assignUser, @kgpai, @pedroerp, @bdice.
As agreed, here is the second part of the change to update to cmake 3.28.
This one uses the cmake support previously added to refactor the cmake modules.

Copy link
Copy Markdown
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good thanks! As the docker images have been updated you can remove the steps where you installed the recent cmake in #9861.

@assignUser assignUser mentioned this pull request May 24, 2024
@assignUser assignUser changed the title Refactor cmake modules Add GCS dependencies to resolve_dependency and refactor cmake modules May 24, 2024
Copy link
Copy Markdown
Collaborator

@assignUser assignUser left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Once CI turns green this should be ready to merge.

@assignUser assignUser added the ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall label May 24, 2024
Copy link
Copy Markdown
Collaborator

@bdice bdice left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This looks good from what I can tell.

@assignUser assignUser added ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall and removed ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall labels May 28, 2024
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kgpai has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator.

The new features in cmake 3.28 allow the cmake modules to be simplified.
@facebook-github-bot
Copy link
Copy Markdown
Contributor

@kgpai merged this pull request in c50a117.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

CLA Signed This label is managed by the Facebook bot. Authors need to sign the CLA before a PR can be reviewed. Merged ready-to-merge PR that have been reviewed and are ready for merging. PRs with this tag notify the Velox Meta oncall

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants