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

Correctly try to open split files. #880

Merged
merged 2 commits into from
Apr 30, 2024
Merged

Correctly try to open split files. #880

merged 2 commits into from
Apr 30, 2024

Conversation

mgautierfr
Copy link
Collaborator

Fix #879

First commit is not really related to this PR but recent gcc complain about the missing default destructor in test/archive.cpp. So I fix this in the same time. If you prefer, I can move it a different PR as it is not straightforward fix.

include/zim/blob.h Outdated Show resolved Hide resolved
src/file_compound.cpp Outdated Show resolved Hide resolved
src/file_compound.cpp Outdated Show resolved Hide resolved
src/fileimpl.h Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
@mgautierfr
Copy link
Collaborator Author

It appears that the first commit is a bit too aggressive (without speaking potential ABI break). See https://github.com/openzim/libzim/actions/runs/8882043713/job/24385796413?pr=880
_data.get() should return a char* (according to this and this as we are cpp17) but it returns a char (*)[] on some platform (probably because they are not c++17 ready).

I have propose another PR #881 simply testing creation of std::shared_ptr<char> in test.
I will removed the first commit of this PR.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

Please edit/clean-up the history of this PR (and rewrite the commit messages as needed).

src/file_reader.cpp Outdated Show resolved Hide resolved
@mgautierfr mgautierfr force-pushed the split_file_open branch 3 times, most recently from 2bdffb0 to e028dc5 Compare April 30, 2024 07:41
@mgautierfr
Copy link
Collaborator Author

Please edit/clean-up the history of this PR (and rewrite the commit messages as needed).

Done

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

A few final touches (that can be made in place/without fixup commits) and we can merge.

src/fileimpl.cpp Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
src/fileimpl.cpp Outdated Show resolved Hide resolved
One is for simple file part only, the other is for multi part only.

We also introduce a static helper function `openSinglePieceOrSplitFile`
to build the FileCompound as before.
This helper function is put in FileImpl which can seem a bit odd but will
simplify next commits.
@mgautierfr
Copy link
Collaborator Author

New version (simpler) following your live advice.

Copy link
Collaborator

@veloman-yunkan veloman-yunkan left a comment

Choose a reason for hiding this comment

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

LGTM with a small reservation that can be addressed before merging - the description of the second commit doesn't accurately describe the change; it would be nice if the change in user observable behaviour (opening of .zimaa files) is mentioned.

…opening.

Now, it tries to open as single piece only if not ending by `.zimaa`.
If it cannot find the file, or if ending with `.zimaa`, it tries multi zim part.

In any case, FileImpl constructor check for size and throw a
ZimFileFormatError is file's size doesn't correspond to what is the header.
@mgautierfr mgautierfr merged commit 1e42f56 into main Apr 30, 2024
34 checks passed
@mgautierfr mgautierfr deleted the split_file_open branch April 30, 2024 20:09
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.

Better opening of split zim archive.
2 participants