Run Github Actions on pull requests #198
Conversation
facebook-github-bot
added
the
CLA Signed
|
(Hope you don't mind this PR! Feel free to treat it as a suggestion that you can turn down if you think it results in too many builds. I figured that the frequency of PRs is much less than the frequency of the scheduled builds that already happen, so in my mind the trade-off was worth it.) |
|
@jiayuebao has imported this pull request. If you are a Meta employee, you can view this diff on Phabricator. |
Summary: Fix OSS builds by adding numa deps to build files. Currently some fail on missing `numa.h`. Context: #161 added the dependencies to the centOS, debian, and ubuntu18 build files. The PR was opened in Sep 2022 but only landed in Dec 2022, and so probably missed out on the fedora, rocky and arch build files which were added in-between those dates. Having had those build actions run on PRs would have caught this (currently, they are only scheduled.) Pull Request resolved: #197 Test Plan: Github Actions builds (ideally, #198 would be landed first.) I've checked that those packages exist for the respective repositories but didn't run them myself. Reviewed By: jaesoo-fb Differential Revision: D43587970 Pulled By: jiayuebao fbshipit-source-id: 8c59e48528042350e576a45ffc3bf2520699f5a9
|
Triggering builds on PR could have avoided the regression fixed by 197. If the build failures had shown up before #161 (a long-running PR) was merged into master, it could have been fixed in PR 161 itself. With only scheduled builds, the regression only showed after 161 landed. I imagine it's common for people to develop against one OS and that it might be helpful to have builds run on PR to catch any accidental breakage of other OSes. No concern to land it first, but 198 would have helped to verify if 197 fixed those builds. |
|
@wonglkd: thank you for sending this change. Yeah I have been reviewing our github build issues and came to the same conclusion. PR needs to trigger builds. (In fact they should run our tests as well but I haven't figured out a way yet) |
|
@wonglkd: From the recent builds https://github.com/facebook/CacheLib/actions: I see both "rockylinux" and "fedora" have passed successfully, so I think the fix of #197 works :) . For this PR, I do see quite a few build failures - some are caused by missing numa deps while others are not, could you double check all these build failures? |
|
@jiayuebao I checked the build failures - they're the same as the scheduled ones. There are at least two separate issues (XDCHECK on CentOS[1], and a Docker issue on Debian[2]), and won't be solved by this PR. I did figure out possible solutions to them but think they will be better addressed by their own PRs - I might open an issue to track all the build failures. Btw, I was thinking that if this is too many builds to trigger, we could still get decent coverage by triggering one of each type (e.g., only CentOS 8.5, only RockyLinux 9.0, etc), but would defer to you re the exact frequency, since I don't have visibility into what build minute budget you have. I'm less clear on the rpm-build issues and it may involve another repo[3] too - but I think it makes sense to fix the builds first, and then figure that out after. [1] I'm running tests on a possible fix in my fork to see if it works - couldn't figure out where in the history it broke though, since it overlapped with the fmt break |
|
@therealgymmy sg! Tests would be great. I saw in #135 that pmem got CacheLib CI working with tests? That could be one direction to look at. |
@wonglkd Got it! Thank you for confirming. Yeah no need to include the fix in this PR. For the rpm-build issues, our internal team is also aware of that and will work on the fix these days. |
|
@therealgymmy This is the key script pmem/CacheLib uses to run tests: Could add something like this as a separate step after the build. Their use of Docker is probably helpful to reduce build time but not essential. |
|
@jiayuebao merged this pull request in df5b9f6. |
Run GitHub action builds on every pull request, in addition to currently daily scheduled runs.
Benefit: avoid accidentally breaking other OS builds. This would have caught #197.
This triggers whenever PRs are opened or when commits are added to a PR.
Frequency of PRs (total of 76 PRs over last 18 months since CacheLib was open-sourced) is much less than frequency of daily scheduled builds, so this shouldn't add too many builds overall.