Skip to content

Fix Debian GitHub build & zstd CMake error - #200

Closed
wonglkd wants to merge 5 commits into
facebook:mainfrom
wonglkd:fix-debian-build
Closed

Fix Debian GitHub build & zstd CMake error#200
wonglkd wants to merge 5 commits into
facebook:mainfrom
wonglkd:fix-debian-build

Conversation

@wonglkd

@wonglkdwonglkd commented Feb 28, 2023

Copy link
Copy Markdown
Contributor
  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-botfacebook-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
@wonglkdwonglkd changed the title Explicitly mark Git repo as safeFix Debian GitHub build: Explicitly mark Git repo as safeFeb 28, 2023
@wonglkd

wonglkd commented Feb 28, 2023

Copy link
Copy Markdown
ContributorAuthor

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).

@wonglkdwonglkd changed the title Fix Debian GitHub build: Explicitly mark Git repo as safeFix Debian GitHub buildFeb 28, 2023
@wonglkdwonglkd mentioned this pull request Feb 28, 2023
4 tasks
@wonglkd

wonglkd commented Feb 28, 2023

Copy link
Copy Markdown
ContributorAuthor

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
Copy Markdown
ContributorAuthor

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

@jaesoo-fbjaesoo-fb left a comment

Copy link
Copy Markdown
Contributor

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.

Comment threadcontrib/build-package.sh Outdated
@wonglkd

wonglkd commented Mar 1, 2023

Copy link
Copy Markdown
ContributorAuthor

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

@wonglkdwonglkd mentioned this pull request Mar 1, 2023
@wonglkdwonglkd changed the title Fix Debian GitHub buildFix Debian GitHub build & zstd CMake errorMar 1, 2023
Comment threadcontrib/build-package.sh Outdated
@wonglkd

Copy link
Copy Markdown
ContributorAuthor

Made requested changes and rebased.

Comment thread.github/workflows/build-cachelib-debian-10.yml Outdated

@jaesoo-fbjaesoo-fb left a comment

Copy link
Copy Markdown
Contributor

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
Copy Markdown
Contributor

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

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

Copy link
Copy Markdown
ContributorAuthor

@jaesoo-fb rebased!

@facebook-github-bot

Copy link
Copy Markdown
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
Copy Markdown
Contributor

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

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

Labels

CLA SignedThis 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

@wonglkd@jaesoo-fb@facebook-github-bot