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

Fix msgpack runtime on big-endian OpenBSD/sparc64 #7181

Merged
merged 5 commits into from
Dec 4, 2022

Conversation

klemensn
Copy link

@klemensn klemensn commented Dec 2, 2022

  • Fallback to compiler defines when __BYTE_ORDER is not available
  • simplify the endianity handling
  • additional cleanup
  • Fix build error caused by ntohs, ntohl

Backport msgpack/msgpack-python#513 (as discussed in
#6149 (comment)) and msgpack/msgpack-python#514.

Applied with

$ gh -R msgpack/msgpack-python pr diff --patch 513 |
	git am --directory src/borg/algorithms/ -
$ gh -R msgpack/msgpack-python pr diff --patch 514 |
	git am --directory src/borg/algorithms/ -

Tested on OpenBSD/sparc64 (big-endian) and OpenBSD/amd64 (little-endian).

@ThomasWaldmann
Copy link
Member

ThomasWaldmann commented Dec 3, 2022

@klemensn Hmm, the CI is unhappy, ntohs and ntohl are not declared?

https://linux.die.net/man/3/ntohl man page says it needs arpa/inet.h.

klemensn added a commit to klemensn/borg that referenced this pull request Dec 3, 2022
This reverts commit 63beb57.

ntohl(3) requires <arpa/inet.h> which got removed from the !WIN32 block.

Found through borgbackup#7181 failing CI
after only applying msgpack-python's patches:

https://github.com/borgbackup/borg/actions/runs/3605243326/jobs/6080079422#step:8:51
```
src/borg/algorithms/msgpack/sysdep.h:90:28: error: ‘ntohs’ was not declared in this scope
   90 | #  define _msgpack_be16(x) ntohs(x)
      |                            ^~~~~
```
@klemensn
Copy link
Author

klemensn commented Dec 3, 2022

@klemensn Hmm, the CI is unhappy, ntohs and ntohl are not declared?

https://linux.die.net/man/3/ntohl man page says it needs arpa/inet.h.

Indeed. I guess this header is pulled in by something else already on OpenBSD.

@klemensn klemensn changed the title Fix msgpack runtime on big-endian Fix msgpack runtime on big-endian OpenBSD/sparc64 Dec 3, 2022
@klemensn
Copy link
Author

klemensn commented Dec 3, 2022

Other big-endian OpenBSD architectures such as macppc and octeon are NOT effected by this, hence the updated PR title.

This is most likely due to the fact that octeon and macppc use clang 13.0.0 from base while sparc64 still uses GCC 8.4.0 from ports to build borgbackup and msgpack-python packages.

@codecov-commenter
Copy link

codecov-commenter commented Dec 3, 2022

Codecov Report

Merging #7181 (9d5631f) into 1.1-maint (9e22b27) will decrease coverage by 0.59%.
The diff coverage is 100.00%.

@@              Coverage Diff              @@
##           1.1-maint    #7181      +/-   ##
=============================================
- Coverage      79.66%   79.06%   -0.60%     
=============================================
  Files             27       27              
  Lines          10595    10595              
  Branches        2172     2172              
=============================================
- Hits            8440     8377      -63     
- Misses          1614     1673      +59     
- Partials         541      545       +4     
Impacted Files Coverage Δ
src/borg/algorithms/msgpack/_version.py 100.00% <100.00%> (ø)
src/borg/xattr.py 61.35% <0.00%> (-20.78%) ⬇️
src/borg/platform/__init__.py 75.00% <0.00%> (-15.00%) ⬇️
src/borg/platform/base.py 78.63% <0.00%> (-3.42%) ⬇️
src/borg/archive.py 81.28% <0.00%> (-0.59%) ⬇️
src/borg/helpers.py 86.58% <0.00%> (-0.21%) ⬇️
src/borg/repository.py 85.12% <0.00%> (-0.20%) ⬇️

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

@ThomasWaldmann
Copy link
Member

Ah, CI is happy again. So I guess msgpack project should also revert that change.

@klemensn
Copy link
Author

klemensn commented Dec 3, 2022

Ah, CI is happy again. So I guess msgpack project should also revert that change.

I've pointed that out already in their PR.
Either way, this wouldn't effect borg 1.1.x, I think, so this backport should be good to go now.

@ThomasWaldmann
Copy link
Member

https://github.com/borgbackup/borg/blob/e79986b2835291431f04683423304a02b039c0c1/src/borg/algorithms/msgpack/_version.py

Can you increase that (...borg1 -> ...borg2) and add a small description there?

Maybe first wait for feedback from msgpack project though.

@klemensn
Copy link
Author

klemensn commented Dec 3, 2022

Ah, CI is happy again. So I guess msgpack project should also revert that change.

They just did through msgpack/msgpack-python#514.
I'll replace my revert with their commit and rewrap the PR.

@ThomasWaldmann
Copy link
Member

OK, looks like this is finished (and CI is happy)?

@klemensn
Copy link
Author

klemensn commented Dec 4, 2022

OK, looks like this is finished (and CI is happy)?

Yup, pretty sure that's it.

@ThomasWaldmann ThomasWaldmann merged commit 9ad9d4b into borgbackup:1.1-maint Dec 4, 2022
@ThomasWaldmann
Copy link
Member

Thanks for fixing this! (such platform stuff is hard to test)

@klemensn
Copy link
Author

klemensn commented Dec 5, 2022

Thanks for fixing this! (such platform stuff is hard to test)

Would you be able to add new platforms to your CI?
Big or little endian, LLVM/Clang or GCC, whatever new combination you can test would increase coverage.
I have no idea how GitHub works in this regard.

@ThomasWaldmann
Copy link
Member

IIRC github actions only has intel/amd CPUs (and linux, macos and windows as OS).

So guess it would need quite some work to do BE testing there, like using kvm to emulate a BE CPU, having some appropriate BE-supporting OS in that VM, ...

Could be also quite slow. For bootstrapping that VM as well as for the borg pytest test run.

@klemensn klemensn deleted the 1.1-maint branch December 25, 2022 10:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants