Skip to content

Fix Debian GitHub build & zstd CMake error #200

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

Closed
wants to merge 5 commits into from

Conversation

wonglkd
Copy link
Contributor

@wonglkd wonglkd commented Feb 28, 2023

  1. Workaround for Debian Docker image bug that is breaking Debian build on GitHub (Explicitly mark Git repo as safe).
  2. Pin zstd to a commit that resolves problems with older CMakes (note: affects all OSes, not just Debian)

Context for 1: In latest Debian Docker image , there is a regression that affects the checkout action.

From actions/checkout#1169:

  • Checkout runs, and runs /usr/bin/git config --global --add safe.directory
  • The global .gitconfig does not exist
  • Any calls to git remain unsafe/dubious

The suggested workaround was to use --system instead of --global.

Test Plan: See if GitHub Action Debian build is fixed.

@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 Feb 28, 2023
@wonglkd wonglkd changed the title Explicitly mark Git repo as safe Fix Debian GitHub build: Explicitly mark Git repo as safe Feb 28, 2023
@wonglkd
Copy link
Contributor Author

wonglkd commented Feb 28, 2023

It fixed the issue above, but the Debian build now fails on the cmake/zstd issue. zstd 1.5.4 was released in Jan-23 but preceding v1.5.2 was in Jan-22 (a whole year in-between), so not sure when we might see the fix (on dev branch) merged into release. We could do what is done by this repo, i.e., cherrypicking the commit:

git fetch origin dev
git cherry-pick 8420502e

Or pin it at 1.5.2 or that commit with the fix (con: need to manually update it later), or use the dev branch (con: potentially less stable than release).

@wonglkd wonglkd changed the title Fix Debian GitHub build: Explicitly mark Git repo as safe Fix Debian GitHub build Feb 28, 2023
@wonglkd wonglkd mentioned this pull request Feb 28, 2023
4 tasks
@wonglkd
Copy link
Contributor Author

wonglkd commented Feb 28, 2023

Decided to pin zstd to the commit that has the cmake fix. Pinning it might be a good idea anyway for stability. Doing it in general and not just for Debian since it affects all systems with an older cmake.

@wonglkd
Copy link
Contributor Author

wonglkd commented Feb 28, 2023

Debian uses gcc-8.3 and thus now fails on the issue fixed by #201.

Copy link
Contributor

@jaesoo-fb jaesoo-fb left a comment

Choose a reason for hiding this comment

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

Thanks for working on this.

@wonglkd
Copy link
Contributor Author

wonglkd commented Mar 1, 2023

Updated as suggested. Thanks! Also, I got a successful Debian build on my fork after I cherrypicked #201: https://github.com/wonglkd/CacheLib-1/actions/runs/4298872004/jobs/7493484190

@wonglkd wonglkd mentioned this pull request Mar 1, 2023
@wonglkd wonglkd changed the title Fix Debian GitHub build Fix Debian GitHub build & zstd CMake error Mar 1, 2023
@wonglkd
Copy link
Contributor Author

wonglkd commented Mar 1, 2023

Made requested changes and rebased.

Copy link
Contributor

@jaesoo-fb jaesoo-fb left a comment

Choose a reason for hiding this comment

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

Thanks for fixing this.

@jaesoo-fb
Copy link
Contributor

Somehow I cannot import and merge this. @wonglkd Just in case, could you rebase?

wonglkd added 5 commits March 2, 2023 00:53
Workaround for Debian Docker bug.
Pin zstd at commit that fixes cmake error (after 1.5.4)
Used external_git_tag instead, added comments, fixed a typo.
@wonglkd wonglkd force-pushed the fix-debian-build branch from 2d74276 to c97b8e7 Compare March 2, 2023 00:53
@wonglkd
Copy link
Contributor Author

wonglkd commented Mar 2, 2023

@jaesoo-fb rebased!

@facebook-github-bot
Copy link
Contributor

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

@facebook-github-bot
Copy link
Contributor

@jaesoo-fb merged this pull request in 185bbe6.

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants