Skip to content
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

use system deps in github CI for fbthrift #595

Closed
wants to merge 1 commit into from

Conversation

ahornby
Copy link
Contributor

@ahornby ahornby commented Mar 13, 2024

use system deps in github CI for fbthrift

Save some time by not rebuilding cmake & boost et al on each CI run

Summary:

Regenerated github actions with:

./build/fbcode_builder/getdeps.py --allow-system-packages generate-github-actions --free-up-disk --src-dir=. --output-dir=.github/workflows fbthrift

Test Plan:

CI

Save some time by not rebuilding cmake & boost et al on each CI run

Summary:

Regenerated github actions with:
```
./build/fbcode_builder/getdeps.py --allow-system-packages generate-github-actions --free-up-disk --src-dir=. --output-dir=.github/workflows fbthrift
```

Test Plan:

CI
@ahornby ahornby marked this pull request as ready for review March 14, 2024 00:10
@ahornby
Copy link
Contributor Author

ahornby commented Mar 18, 2024

cc @vitaut looks like it speeds up GitHub CI

- name: Update system package info
run: sudo apt-get update
- name: Install system deps
run: sudo python3 build/fbcode_builder/getdeps.py --allow-system-packages install-system-deps --recursive fbthrift
Copy link
Contributor

Choose a reason for hiding this comment

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

This part worries me. I don't think we want to install system thrift when building thrift.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This part worries me. I don't think we want to install system thrift when building thrift.

none of the getdeps manifest request a system dep for thrift, and not of their transient deps do either. Even it if was accidentally added somehow (e.g. someone manually installs ubuntu thrift-compiler), I think still good as the topo-sorted getdeps paths are added in front of the various _PATH env vars.

For further reassurance, not had an issue of folly system packages being found rather than the getdeps built ones

Copy link
Contributor

Choose a reason for hiding this comment

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

Nevermind, I incorrectly assumed that this installs a system version of thrift itself in addition to dependencies.

@vitaut
Copy link
Contributor

vitaut commented Apr 9, 2024

So this bring the build time from ~38 to ~24 minutes? This is a nice improvement but I wonder if we can miss some build issues if we install dependencies instead of building with our own configs?

@ahornby
Copy link
Contributor Author

ahornby commented Apr 15, 2024

So this bring the build time from ~38 to ~24 minutes? This is a nice improvement but I wonder if we can miss some build issues if we install dependencies instead of building with our own configs?

@vitaut its already done for folly, I think its low risk to do fbthrift CI as well. The instructions from README usage suggest using the system deps, so CI will now test that more accurately

@vitaut
Copy link
Contributor

vitaut commented Apr 15, 2024

I'm fine with this change but you need to import it into fbsource.

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@ahornby merged this pull request in bcac89f.

facebook-github-bot pushed a commit to facebook/hhvm that referenced this pull request Apr 16, 2024
Summary:
use system deps in github CI for fbthrift

Save some time by not rebuilding cmake & boost et al on each CI run

Regenerated github actions with:
```
./build/fbcode_builder/getdeps.py --allow-system-packages generate-github-actions --free-up-disk --src-dir=. --output-dir=.github/workflows fbthrift
```

X-link: facebook/fbthrift#595

Reviewed By: vitaut, bigfootjon

Differential Revision: D56162839

Pulled By: ahornby

fbshipit-source-id: d0ed1ef779c839481e0903696e03e1e5624e08df
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.

3 participants