Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. ❌ Your project status has failed because the head coverage (59.70%) is below the target coverage (90.00%). You can increase the head coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #508 +/- ##
=======================================
Coverage 59.70% 59.70%
=======================================
Files 67 67
Lines 10487 10487
Branches 1803 1803
=======================================
Hits 6261 6261
Misses 3531 3531
Partials 695 695
🚀 New features to boost your workflow:
|
artemgavrilov
force-pushed
the
macos
branch
2 times, most recently
from
00ebb28 to
d549677
Compare
| #ifdef __APPLE__ | ||
| #define TMPFS_DIRECTORY "/tmp" | ||
| #else |
There was a problem hiding this comment.
Thing to discuss: It seems that we should check this in runtime, as /dev/shm may be unavailable in some linux distros.
There was a problem hiding this comment.
Yeah, I am not sure about what we should do here.
There was a problem hiding this comment.
One option would be to make it a CLI argument. The issue is that there is no standardized location for temporary files on UNIX which is guaranteed to be in-momory.
| build: | ||
| name: Build | ||
| runs-on: ${{ inputs.os }} | ||
| timeout-minutes: 10 |
There was a problem hiding this comment.
Why was this necessary? The commit message should tell us why or a comment should.
There was a problem hiding this comment.
Just a good practice. During debug I faced hanged tests that consumed a lot of CI time, so timeout here is just a fuse.
| name: Test PSP with TDE enabled by default | ||
| runs-on: ${{ inputs.os }} | ||
| needs: build | ||
| timeout-minutes: 30 |
There was a problem hiding this comment.
Wow ... this is a long timeout.
There was a problem hiding this comment.
For some reason tests much slower on MacOS, I assume because of different hardware.
| if [[ "$OSTYPE" == "linux-gnu"* ]]; then | ||
| ARGS+=" --with-liburing" | ||
| NCPU=$(nproc) | ||
| elif [[ "$OSTYPE" == "darwin"* ]]; then |
There was a problem hiding this comment.
This should be explained in a comment, or commit message. I assume this is because Mac's version of make does not support automatic selection of worker pool.
There was a problem hiding this comment.
I'm not sure that GNU Make has auto selection as well, at least they don't mention this in docs: https://www.gnu.org/software/make/manual/make.html
| #ifdef __APPLE__ | ||
| #define TMPFS_DIRECTORY "/tmp" | ||
| #else |
There was a problem hiding this comment.
Yeah, I am not sure about what we should do here.
| SHLIB_LINK = -lcurl -lcrypto -lssl | ||
| LDFLAGS_EX = -Lsrc/fe_utils -lcurl -lcrypto -lssl -lz -lzstd -llz4 -lpgfeutils $(libpq_pgport) | ||
| SHLIB_LINK += -lcurl -lcrypto -lssl | ||
| LDFLAGS_EX += -Lsrc/fe_utils -lcurl -lcrypto -lssl -lz -lzstd -llz4 -lpgfeutils $(libpq_pgport) |
There was a problem hiding this comment.
These things were intentional, but it is possible that I was wrong. So can you explain in a commit message why this change was necessary?
There was a problem hiding this comment.
Added. Long story short for MacOS extension built as a bundle and linker expects --bundle-loader flag that specifies executable that will load bundle to check symbols against it. Global makefile provides this flag and as I understood it's expected that flags from global makefile are forwarded to extension.
There was a problem hiding this comment.
BTW macos problem was cause just by SHLIB_LINK, so I can limit change only with this varable. But what was the reason to not forward flags from global makefile?
Global makefile provides system spevific falgs. For example for MacOS it provides -bundle_loader flag for linker that speicfies executable that will load an extension. Without that flag linker fails as it cannot check defined symbols.
The liburing in linux specific library, so include it only for linux tartget builds.
Without such limit make will spawn as much jobs as possible. It can harm perfomance and hit proccess number limit in CI environment.
MacOS by default goes with bash 3.x while -v supported since version 4.2.
MacOS doesn't have /dev/shm, so fallback to /tmp instead.
MacOS has different error message for write attempt without permission. While linux says "Permission denied", MacOS says "Read-only file system".
Setup build and test CI workflows for MacOS. These workflows are needed only to be sure that this project builds/works in development environments. So we can omit different build variations.
Reasonable timeouts help to save CI time when some steps hung.
3 participants
Changes made: