stachenov / quazip

Qt/C++ wrapper over minizip
Other
582 stars 232 forks source link

Add BZIP to quazip #150

Closed brunoais closed 1 year ago

brunoais commented 2 years ago

Closes #147

brunoais commented 2 years ago

@stachenov It might be that that's all that was required to do. I'm not fully sure. How do I run the tests?

I have already run:

mkdir build && cd $_
cmake -DQUAZIP_ENABLE_TESTS=ON ..
brunoais commented 2 years ago

Seems like the workflow has what I need... Now I need to add a test for BZIP2

brunoais commented 2 years ago

@stachenov When you have time, please look at this PR. My limited knowledge makes me stumble and fail to achieve the installation.

Note: Do ignore the CI test branch and CI test 20.04 commits. They are just so the CI runs for additional testing on github to make sure it's working without overwhelming my test tokens with other testing environments.

stachenov commented 2 years ago

Finally got to it.

I'm having a hard time trying to compile this on Windows. Fails with

libbzip2.a(compress.c.obj):compress.c:(.text+0x1e35): undefined reference to `bz_internal_error'
libbzip2.a(compress.c.obj):compress.c:(.text+0x23aa): undefined reference to `bz_internal_error'
libbzip2.a(compress.c.obj):compress.c:(.text+0x28db): undefined reference to `bz_internal_error'
libbzip2.a(compress.c.obj):compress.c:(.text+0x28f8): undefined reference to `bz_internal_error'
libbzip2.a(decompress.c.obj):decompress.c:(.text+0x2008): undefined reference to `bz_internal_error'
libbzip2.a(decompress.c.obj):decompress.c:(.text+0x2014): more undefined references to `bz_internal_error' follow

Strangely enough, googling for this error yields just one page of search results, with only one of them being remotely relevant, though entirely useless.

I used to be pretty good at combating these kinds of errors, so maybe I can deal with them when I have more time.

brunoais commented 2 years ago

I used to be pretty good at combating these kinds of errors, so maybe I can deal with them when I have more time.

Thanks for the update, though. Apparently it's not as easy as I thought...

stachenov commented 2 years ago

Linking was never easy on Windows, so nothing new here. Maybe I'll try with Linux, but I'll have to set up a Linux VM first.

brunoais commented 2 years ago

but I'll have to set up a Linux VM first.

If you use windows, just use WSL2. It's way easier having it managed by the OS than you managing it. https://learn.microsoft.com/en-us/windows/wsl/install That's what I do at work. I think that link explains enough. You can get the distro of choice from the windows store.

stachenov commented 2 years ago

Yeah, you're right! I also set it up for work not so long ago, but totally forgot about it since (I only needed it to check one issue). Thanks for reminding me!

stachenov commented 2 years ago

Well, surprisingly, it turns out this is one of the few cases when that error actually means what it's supposed to mean: this function is not defined and it's the client's responsibility to define it. This is also a typical case of RTFM, as I've found it right from the bzip2's manual.

This is a rather weird way to design APIs and leaves me a bit confused. If I provide such a function in QuaZip, it'll conflict with the same function provided by the authors of the QuaZip's client, if they provide any, of course. Or with some other dependency that also happens to use bzip2. Guess I'll have to make it optional then.

brunoais commented 2 years ago

So... It's up for the caller somewhere in the call stack to define the bz_internal_error function?

I guess if so, then it has to be optional. AFAIK, there's no way to do a "create function if not refined", unfortunately... Thank you for the update. Hopefully, I'll be able to use bzip with quazip by the end of the year. If not... Well, you are doing for free so it is as it is.

Just in case: I saw this: https://www.cs.cmu.edu/afs/cs/project/pscico-guyb/realworld/99/code/bzip2-0.9.5c/manual_3.html

However, bz_internal_error appears to only happen when not using stdio. @stachenov , is that your case?

stachenov commented 2 years ago

Ah, I missed the fact that the stdio-free thing is optional.

You actually enabled it yourself by adding this line:

target_compile_definitions(bzip2 PRIVATE -DBZ_NO_STDIO)

Hmm, but then we still need to provide this as an option to users, so there are three cases: either stdio is used, or the user provides the callback, or QuaZip does it. I think I got it now.

I sure hope I'll be able to do it by the end of the year as well. I have more and more free time lately as things settle down, and it's not much work really, so there's a high chance that I'll manage.

brunoais commented 2 years ago

Ah, I missed the fact that the stdio-free thing is optional.

You actually enabled it yourself by adding this line:

target_compile_definitions(bzip2 PRIVATE -DBZ_NO_STDIO)

Ups 🤦 . My bad... I didn't get those errors so I never traced back. I'm too new to CMAKE to be able to notice that. Glad you did. Thanks.

Hmm, but then we still need to provide this as an option to users, so there are three cases: either stdio is used, or the user provides the callback, or QuaZip does it. I think I got it now.

🎉 Awesome!

I sure hope I'll be able to do it by the end of the year as well. I have more and more free time lately as things settle down, and it's not much work really, so there's a high chance that I'll manage.

That's good news! Thanks.

brunoais commented 2 years ago

@stachenov: Any news? 🙂

stachenov commented 2 years ago

Not yet, sadly. Still don't have much free time with this new life :-) Hopefully I can still do it this year.

brunoais commented 2 years ago

Not yet, sadly. Still don't have much free time with this new life :-) Hopefully I can still do it this year.

OK 👍! Thank you for the fast answer. Seems like it's harder than I thought 😢

stachenov commented 2 years ago

I'm pretty sure it won't be that hard if I ever find at least a couple of free hours.

Maybe once winter finally sets in. Because with weather being as awesome as it is, when I'm not working and not shopping, I'm exploring the island with my wife, and when I get back, I'm too tired to do anything.

brunoais commented 2 years ago

Living a life is important too 😉 . I'll wait for such time then!

stachenov commented 1 year ago

Finally got around to this, though with this fine weather I'm not even sure I'll be able to continue maintaining this :-)

For now just enabled STDIO and made some adjustments to make tests work. Please check this out, and if it works, I'll add non-stdio error handling and make another release.

brunoais commented 1 year ago

I'm not even sure I'll be able to continue maintaining this :-)

You won't be able to maintain quazip? Or the BZIP compression?

For now just enabled STDIO and made some adjustments [...] Please check this out

OK

stachenov commented 1 year ago

I mean the whole project. It's not that much time consuming, but life here is too chill even for this. But I'll try to do my best :-)

brunoais commented 1 year ago

But I'll try to do my best

Thank you.

I tried. It seems to work for me.

Thank you very much. I wish I could have been more helpful. My lack of experience with these tools made it so I couldn't do more of that work.

brunoais commented 1 year ago

@stachenov 🤩