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

remove endianness macro, fixes #6105 #6149

Merged

Conversation

ThomasWaldmann
Copy link
Member

thanks to everybody who helped with this, esp. @th0ma7 and @methane.

thanks to everybody who helped with this, esp. @th0ma7 and @methane.
@codecov-commenter
Copy link

codecov-commenter commented Jan 22, 2022

Codecov Report

Merging #6149 (744d82b) into 1.1-maint (9abd749) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           1.1-maint    #6149   +/-   ##
==========================================
  Coverage      80.00%   80.00%           
==========================================
  Files             27       27           
  Lines          10479    10479           
  Branches        2146     2146           
==========================================
  Hits            8384     8384           
  Misses          1575     1575           
  Partials         520      520           

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9abd749...744d82b. Read the comment docs.

@ThomasWaldmann ThomasWaldmann merged commit e41d8e8 into borgbackup:1.1-maint Jan 22, 2022
@ThomasWaldmann ThomasWaldmann deleted the fix-msgpack-endianness branch January 22, 2022 14:37
@klemensn
Copy link

klemensn commented Dec 2, 2022

This broke 1.1.18 on OpenBSD/sparc64, both 7.2-release and 7.2-CURRENT:

$ sysctl kern.version hw.byteorder
kern.version=OpenBSD 7.2-current (GENERIC.MP) #1541: Sun Nov 27 12:27:59 MST 2022
    deraadt@sparc64.openbsd.org:/usr/src/sys/arch/sparc64/compile/GENERIC.MP

hw.byteorder=4321
$ pkg_info -m | grep borg
borgbackup-1.1.18p1 deduplicating backup program
$ borg --version
1.1.18

Initialising a repository shows a message (unclear whether this is a warning, an error or perhaps an expected message):

$ borg init -e none ./test
Unknown integrity data version 0 in integrity.1
$ echo $?
0

Accessing it then fails:

$ borg list ./test
Unknown integrity data version 0 in integrity.1
Local Exception
Traceback (most recent call last):
  File "/usr/local/lib/python3.10/site-packages/borg/archiver.py", line 4846, in main
    exit_code = archiver.run(args)
  File "/usr/local/lib/python3.10/site-packages/borg/archiver.py", line 4778, in run
    return set_ec(func(args))
  File "/usr/local/lib/python3.10/site-packages/borg/archiver.py", line 164, in wrapper
    kwargs['manifest'], kwargs['key'] = Manifest.load(repository, compatibility)
  File "/usr/local/lib/python3.10/site-packages/borg/helpers.py", line 389, in load
    raise ValueError('Invalid manifest version')
ValueError: Invalid manifest version

Platform: OpenBSD [redacted-hostname] 7.2 GENERIC.MP#1541 sparc64
Borg: 1.1.18  Python: CPython 3.10.8 msgpack: 0.5.6.+borg1
PID: 52736  CWD: /home/kn
sys.argv: ['/usr/local/bin/borg', 'list', './test']
SSH_ORIGINAL_COMMAND: None

Purging any local borg state (or trying a fresh OpenBSD installation) yields the same error:

$ rm -rf ./test ~/.cache/borg/ ~/.config/borg/
$ borg --debug init --encryption repokey ./test
using builtin fallback logging configuration
35 self tests completed in 0.95 seconds
Initializing repository at "test"
Encryption NOT enabled.
Use the "--encryption=repokey|keyfile" to enable encryption.
check_free_space: few segments, not requiring a full free segment
check_free_space: calculated working space for compact as 0 bytes
check_free_space: required bytes 169138, free bytes 2987718656
security: initializing unencrypted repository
security: saving state for c73159b5c1d40be48590ba5bacb3d1e80593fe8e2d8655776b0a7f6b1089e049 to /home/kn/.config/borg/security/c73159b5c1d40be48590ba5bacb3d1e80593fe8e2d8655776b0a7f6b1089e049
security: current location   /home/kn/test
security: key type           2
security: manifest timestamp 2022-12-02T17:57:32.805203
security: read previous location '/home/kn/test'
security: read manifest timestamp '2022-12-02T17:57:32.805203'
security: determined newest manifest timestamp as 2022-12-02T17:57:32.805203
security: repository checks ok, allowing access
Synchronizing chunks cache...
Archives: 0, w/ cached Idx: 0, w/ outdated Idx: 0, w/o cached Idx: 0.
Unknown integrity data version 0 in integrity.1
Done.
security: saving state for c73159b5c1d40be48590ba5bacb3d1e80593fe8e2d8655776b0a7f6b1089e049 to /home/kn/.config/borg/security/c73159b5c1d40be48590ba5bacb3d1e80593fe8e2d8655776b0a7f6b1089e049
security: current location   /home/kn/test
security: key type           2
security: manifest timestamp 2022-12-02T17:57:32.805203

Trying to push into this repository via borg create over SSH would not show any error and just block endlessly.

1.2.2 is equally effected.

Reverting https://github.com/borgbackup/borg/pull/6149.patch on top of a pristine 1.1.18 build fixes it:

$ borg init -e none ./testrepo-1.1.18-pr6149_reverted
$ echo $?
0
$ borg list ./testrepo-1.1.18-pr6149_reverted
$ echo $?
0

@ThomasWaldmann
Copy link
Member Author

@klemensn so that platform is big-endian?

@klemensn
Copy link

klemensn commented Dec 2, 2022

@klemensn so that platform is big-endian?

Yes.
https://www.openbsd.org/sparc64.html
https://en.wikipedia.org/wiki/SPARC

@ThomasWaldmann
Copy link
Member Author

ThomasWaldmann commented Dec 2, 2022

about borg 1.1.x: it is pretty much out of maintenance, 1.1.18 was the last release.

about borg 1.2.x:

msgpack is not bundled any more (and thus not built as part of the borg build process), but just listed as requirement, so pip will install it.

so, if you can reproduce the problem with a borg 1.2.x installation, that means that the msgpack python package is broken (like it is not being built correctly on sparc64) and a bug or better a PR should be filed there.

https://github.com/msgpack/msgpack-python

@ThomasWaldmann
Copy link
Member Author

ThomasWaldmann commented Dec 2, 2022

a general issue with BE platforms is the lack of testing infrastructure: I don't have any, so I can't test this.

ideally, there would be a vagrant box that could be used for testing (within a VM), but I don't know of any.

@ThomasWaldmann
Copy link
Member Author

@klemensn btw, my change here was only reflecting that change: https://github.com/msgpack/msgpack-python/pull/495/files

@ThomasWaldmann
Copy link
Member Author

ThomasWaldmann commented Dec 2, 2022

https://github.com/msgpack/msgpack-python/pull/513/files this is a more recent change, check it.

Notably that change is not in a msgpack release as of today.

@klemensn
Copy link

klemensn commented Dec 2, 2022

about borg 1.1.x: it is pretty much out of maintenance, 1.1.18 was the last release.

Good to know, thanks.

about borg 1.2.x:

msgpack is not bundled any more (and thus not built as part of the borg build process), but just listed as requirement, so pip will install it.

I noticed that 1.1.x bundles msgpack while 1.2.x uses the system version, like the OpenBSD port does.

so, if you can reproduce the problem with a borg 1.2.x installation, that means that the msgpack python package is broken (like it is not being built correctly on sparc64) and a bug or better a PR should be filed there.

https://github.com/msgpack/msgpack-python

The latest borgbackup-1.2.2p1 and borgbackup-1.1.18p1 packages on OpenBSD/sparc64 fail equally.

Just as an additional data point: OpenBSD packages the latest msgpack-python as py3-msgpack-1.0.4p1v0 and its regression tests all pass:
https://gist.github.com/936496c1bea568a0f741ae9b3ec54d39

@ThomasWaldmann
Copy link
Member Author

ThomasWaldmann commented Dec 2, 2022

@klemensn can you uninstall the current msgpack you use with borg 1.2 and then:

pip install git+https://github.com/msgpack/msgpack-python.git@master

after that, install borg 1.2 and try it again (hopefully it take that msgpack and does not install a release over it)

would that be the same as the "1.0.4p1v0" you use?

@klemensn
Copy link

klemensn commented Dec 2, 2022

@klemensn can you uninstall the current msgpack you use with borg 1.2 and then:

pip install git+https://github.com/msgpack/msgpack-python.git@master

after that, install borg 1.2 and try it again (hopefully it take that msgpack and does not install a release over it)

Thanks, I'll simply pull the upstream patches into the proper OpenBSD net/py-msgpack port/package and see if that fixes borg 1.2.2.

@ThomasWaldmann
Copy link
Member Author

btw, borg checks the msgpack version and only accepts known-good versions (1.0.4 should be ok, but anything higher needs a change in borg also).

@klemensn
Copy link

klemensn commented Dec 2, 2022

btw, borg checks the msgpack version and only accepts known-good versions (1.0.4 should be ok, but anything higher needs a change in borg also).

Yes, but cherry-picking such upstream commits into the port/package won't change its version (unless I crank more than what's known as REVISION (incremented for minor stuff like homepage/maintainer/etc. changes, then dropped again on new upstream versions).

@ThomasWaldmann
Copy link
Member Author

ok, good. btw, if you like, make a PR against borg 1.1-maint with whatever fix you find.

likely there won't be another borg 1.1.x release, but at least the branch then has all the patches some other BE user might need.

@klemensn
Copy link

klemensn commented Dec 2, 2022

Applying https://github.com/msgpack/msgpack-python/pull/513.patch to net/py-msgpack and making sysutils/borgbackup/1.2 requiring that specific fixed py3-msgpack version fixes it.

So nothing to do for borgbackup 1.2.x.

msgpack-python could roll another release with this fix and should probably expand their regression tests to cover this.

I'll take care of the OpenBSD ports, fixed binary packages should roll out soon.

@klemensn
Copy link

klemensn commented Dec 2, 2022

ok, good. btw, if you like, make a PR against borg 1.1-maint with whatever fix you find.

Since reverting this PR fixes things on OpenBSD, I'm inclined to do just that for our port and call it a day. (after ensuring that other big-endian as well as little-endian architectures still work).

likely there won't be another borg 1.1.x release, but at least the branch then has all the patches some other BE user might need.

I'll leave that to someone with enough time and energy to extract a proper fix out of this revert.

@ThomasWaldmann
Copy link
Member Author

I don't think a revert of that changeset would be the right change for borg 1.1.x, considering that this changeset was done for a reason.

But rather the later change from msgpack master should be applied to the msgpack code we have bundled in borg 1.1.x:

https://patch-diff.githubusercontent.com/raw/msgpack/msgpack-python/pull/513.patch

@klemensn
Copy link

klemensn commented Dec 2, 2022

I don't think a revert of that changeset would be the right change for borg 1.1.x, considering that this changeset was done for a reason.

But rather the later change from msgpack master should be applied to the msgpack code we have bundled in borg 1.1.x:

https://patch-diff.githubusercontent.com/raw/msgpack/msgpack-python/pull/513.patch

Right, that should also work.

@ThomasWaldmann
Copy link
Member Author

Fixed by #7181 in 1.1-maint.

The fix for borg 1.2 and 2.0 will come automatically via a not-yet released msgpack-python 1.0.5.

@klemensn
Copy link

klemensn commented Dec 5, 2022

Fixed by #7181 in 1.1-maint.

The fix for borg 1.2 and 2.0 will come automatically via a not-yet released msgpack-python 1.0.5.

Or you apply it to OS packages: borgbackup-1.1.18p3, py3-msgpack-1.0.4p2v0/borgbackup-1.2.2p3.
I've also backported this to 7.2, so stable packages will be available soon for aarch64, amd64, i386 and sparc64.

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.

3 participants