dockerfiles: adding support for linux-ppc64le #6792
Conversation
adilhusain-s
requested review from
niedbalski,
patrick-stephens,
fujimotos,
edsiper,
leonardo-albertovich and
koleini
as code owners
adilhusain-s
force-pushed
the
ppc64le-ci
branch
from
5039abe to
b065f61
Compare
src/flb_utils.c
Outdated
| @@ -706,7 +706,10 @@ int flb_utils_write_str(char *buf, int *off, size_t size, | |||
| encoded_to_buf(p, tmp, len); | |||
| p += len; | |||
| } | |||
| else if (c >= 0x80 && c <= 0xFFFF) { | |||
| #if defined(__powerpc64__) | |||
There was a problem hiding this comment.
This is the only part of this PR I want to understand better before approving it, I think I asked about it in the previous version but there was no response, could you explain why is this change necessary?
There was a problem hiding this comment.
Hi @leonardo-albertovich
I remember you asked for this before. When I tried last time I could not get to the root of it in the limited time that was available. I made this change after the test failed, based on the actual value of c being received for Power. I will look at it again.
There was a problem hiding this comment.
Thank you, getting to the root of this is the only think missing from my point of view, if you need any help with it let me know, I'd be glad to assist you as much as possible.
There was a problem hiding this comment.
Hi @leonardo-albertovich
The issue is plain characters are signed on x86_64, and unsigned on ppc64le. Forcing chars on ppc64le to be signed using the gcc option -fsigned-char. So no change required to the original flb_utils.c. Will update the PR shortly.
There was a problem hiding this comment.
Awesome, with that out of the way I think it's good to go as long as it runs on CI and passes the tests of course.
Edit: Thank you very much for looking into it.
adilhusain-s
force-pushed
the
ppc64le-ci
branch
from
b065f61 to
3ca942e
Compare
|
Just need to make sure the commits are signed and use the right prefix: https://github.com/fluent/fluent-bit/blob/master/CONTRIBUTING.md#commit-changes We should also ensure it builds across all targets with the |
adilhusain-s
force-pushed
the
ppc64le-ci
branch
from
3ca942e to
9ef9cc2
Compare
patrick-stephens
temporarily deployed
to
unstable
GitHub Actions
Inactive
patrick-stephens
temporarily deployed
to
pr-package-test
GitHub Actions
Inactive
patrick-stephens
temporarily deployed
to
pr-package-test
GitHub Actions
Inactive
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
This PR is stale because it has been open 45 days with no activity. Remove stale label or comment or this will be closed in 10 days. |
|
I think there are still some outstanding review comments to address from @leonardo-albertovich |
|
let's review this @leonardo-albertovich |
|
I need to investigate the ol' PR fork pushing images failures but fundamentally the images build - they just fail to upload with 403 errors for permissions (I'm guessing because it is from a fork). |
This is why: https://github.blog/changelog/2021-04-20-github-actions-control-permissions-for-github_token/ It means we cannot push images for PRs run via forks. |
|
Once #10106 is merged we can rebase this PR to confirm all is green with container builds now. |
|
Hi @patrick-stephens Let us know if you need our help. cc: @seth-priya |
|
@adilhusain-s could you rebase from |
|
@patrick-stephens My bad, I have messed up the branch while trying to rebase. Looks like a new PR is the only option now. |
Successfully tested the change on my fork.
workflow run link