[native] Ensure inheritance of fbos version for proxygen

czentgr commented 3 months ago

When the setup scripts from velox are sourced the FB_OS_VERSION is inherited. Proxygen should use the version of the FBOS dependencies.

This also removes the need for additional changes when the Velox submodule is updated.

Description

Motivation and Context

Impact

Test Plan

Contributor checklist

[x] Please make sure your submission complies with our development, formatting, commit message, and attribution guidelines.
[x] PR description addresses the issue accurately and concisely. If the change is non-trivial, a GitHub Issue is referenced.
[x] Documented new properties (with its default value), SQL syntax, functions, or other functionality.
[x] If release notes are required, they follow the release notes guidelines.
[x] Adequate tests were added if applicable.
[x] CI passed.

Release Notes

Please follow release notes guidelines and fill in the release notes below.

== NO RELEASE NOTE ==

czentgr commented 3 months ago

@majetideepak FYI

czentgr commented 3 months ago

@czentgr Thanks! Were you able to verify that the correct value is being inherited locally?

Yes, I was able to verify. Initially, I wasn't so sure myself if sourcing would preserve the variable from the respective velox setup script. So I did the test prior to opening the PR.

output:

+++ dirname ../presto/presto-native-execution/scripts/../velox/scripts/setup-macos.sh
++ SCRIPTDIR=../presto/presto-native-execution/scripts/../velox/scripts
++ source ../presto/presto-native-execution/scripts/../velox/scripts/setup-helper-functions.sh
+++ getconf _NPROCESSORS_ONLN
++ NPROC=10
+++ pwd
++ DEPENDENCY_DIR=/Users/czentgr/gitspace/deps
++ MACOS_VELOX_DEPS='flex bison protobuf@21 icu4c boost gflags glog libevent lz4 lzo snappy xz zstd openssl libsodium'
++ MACOS_BUILD_DEPS='ninja cmake ccache'
++ FB_OS_VERSION=v2024.05.20.00
++ return
++ return
+ [[ 1 -ne 0 ]]
+ for cmd in '"$@"'
+ run_and_time install_proxygen
+ install_proxygen
+ github_checkout facebook/proxygen v2024.05.20.00
+ local REPO=facebook/proxygen

czentgr commented 2 months ago

@majetideepak I've rebased to bring it up to date. Is it possible we can merge this or are we waiting for another review?

majetideepak commented 2 months ago

@czentgr This is good to merge. I approved it but missed merging it in the past. Thanks for the rebase. I will land after CI finishes.

prestodb / presto