Skip to content
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

builtin_davix: Drop PATCH_COMMAND #16802

Merged
merged 1 commit into from
Nov 4, 2024
Merged

Conversation

hahnjo
Copy link
Member

@hahnjo hahnjo commented Oct 31, 2024

The build fix for FreeBSD is included already since DAVIX 0.8.6, so the PATCH_COMMAND is not needed (it was merged because the PR was older, opened in February, than the upgrade of the builtin in May). In fact the command breaks builtin_davix on Linux systems because sed -i is not portable between GNU sed and BSD sed. (Note that the same is true for the patch itself, see my comment in cern-fts/davix#113 (comment).)

The build fix for FreeBSD is included already since DAVIX 0.8.6, so
the PATCH_COMMAND is not needed (it was merged because the PR was
older, opened in February, than the upgrade of the builtin in May).
In fact the command breaks builtin_davix on Linux systems because
`sed -i` is not portable between GNU sed and BSD sed.
(Note that the same is true for the patch itself, see my comment in
cern-fts/davix#113 (comment)
.)
@hahnjo hahnjo requested a review from guitargeek October 31, 2024 13:30
@hahnjo hahnjo self-assigned this Oct 31, 2024
@hahnjo hahnjo requested a review from bellenot as a code owner October 31, 2024 13:30
Copy link

Test Results

    17 files      17 suites   3d 6h 56m 0s ⏱️
 2 663 tests  2 663 ✅ 0 💤 0 ❌
43 575 runs  43 575 ✅ 0 💤 0 ❌

Results for commit 7067264.

Copy link
Contributor

@guitargeek guitargeek left a comment

Choose a reason for hiding this comment

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

LGTM, thanks!

@hahnjo hahnjo merged commit fe1e80b into root-project:master Nov 4, 2024
20 of 21 checks passed
@hahnjo hahnjo deleted the builtin_davix branch November 4, 2024 07:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants