- 
                Notifications
    
You must be signed in to change notification settings  - Fork 5.5k
 
build(native): Refactor setup scripts #26406
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
          
Reviewer's guide (collapsed on small PRs)Reviewer's GuideThis PR refactors the native setup scripts to download released tarball artifacts instead of cloning from Git, parameterizes dependency versions, updates build helper invocations to use cmake_install_dir, and switches CI workflows to a custom container image. Sequence diagram for dependency installation in setup scripts (refactored)sequenceDiagram
    participant Script
    participant SourceRepo
    participant TarballServer
    participant BuildHelper
    Script->>TarballServer: Download dependency tarball
    Script->>BuildHelper: Extract and install from tarball
    BuildHelper->>Script: Installation complete
    Class diagram for build helper function changesclassDiagram
    class Script {
      +install_gperf()
      +install_proxygen()
      +install_jwt_cpp()
      +install_prometheus_cpp()
    }
    class BuildHelper {
      +wget_and_untar(url, target_dir)
      +cmake_install_dir(target_dir, options)
    }
    Script --> BuildHelper: uses
    Script : -github_checkout() (removed)
    Script : -cmake_install() (replaced)
    Script : +cmake_install_dir() (added)
    Script : +wget_and_untar() (added)
    Script : +parameterized dependency versions
    File-Level Changes
 Tips and commandsInteracting with Sourcery
 Customizing Your ExperienceAccess your dashboard to: 
 Getting Help
  | 
    
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
| wget ${WGET_OPTIONS} https://mirrors.ocf.berkeley.edu/gnu/gperf/gperf-3.1.tar.gz && | ||
| tar -xzf gperf-3.1.tar.gz && | ||
| cd gperf-3.1 && | ||
| wget_and_untar https://mirrors.ocf.berkeley.edu/gnu/gperf/gperf-${GPERF_VERSION}.tar.gz gperf | 
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I remember @PingLiuPing also has a fix relate to this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, it is in setup-macos.sh. I think it is ok to just use this site for downloading.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me update setup-macos.sh as well with this URL.
A recent change in Velox changed what the current directory is after github_checkout. This breaks building the PrestoC++ dependencies. The refactor removes github cloning and instead pulls released tar.gz images of the source code. This also increases the download speed.
e0bbdee    to
    229800d      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @czentgr. Looking forward to this fix :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
| 
           Removing the custom dependency image, the runs were clean. There is another PR updating these.  | 
    
        3f6da52
      
    229800d    to
    3f6da52      
    Compare
  
    There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks @czentgr
Description
This is a companion PR to #26385
Motivation and Context
Impact
Test Plan
Contributor checklist
Release Notes
Please follow release notes guidelines and fill in the release notes below.