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

Use minizip to replace libgsf #1003

Merged
merged 1 commit into from
Apr 17, 2020

Conversation

yetist
Copy link
Contributor

@yetist yetist commented Apr 17, 2020

The previous closed PR, see #975

@yetist
Copy link
Contributor Author

yetist commented Apr 17, 2020

CI is disabled?

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

This is much better than the previous iteration, thank you. Minizip seems like a reasonable dependency—I don't know about other platforms but at least for Arch Linux it is in the [core] repository so it is an easy dependency to build with.

@yetist
Copy link
Contributor Author

yetist commented Apr 17, 2020

minizip is available in most distro, see https://pkgs.org/download/minizip

@karlkleinpaste
Copy link
Contributor

this one i like a whole lot more. simple and direct. thanx.

@karlkleinpaste karlkleinpaste merged commit 6abeb8d into crosswire:master Apr 17, 2020
@karlkleinpaste
Copy link
Contributor

ok, i'm a little bit bothered now. by the time you submit a pull req, i assume that you've done some testing of your own, starting with successful build. i'm on up to date F31.
screenshot1
zip.h only includes mz_compat.h, which does not know these symbols.

@karlkleinpaste
Copy link
Contributor

in fact, they're not known at all.

$ grep Z_BEST *
$ grep Z_DEFL *
$

@greg-hellings
Copy link
Contributor

https://travis-ci.org/github/crosswire/xiphos/builds/676272298 CI was never updated for this PR, either. I don't know why Travis didn't report here in the builds, but the build did execute and failed (missing the new dep, not the same error as you're seeing @karlkleinpaste)

@greg-hellings
Copy link
Contributor

Those symbols exist in /usr/include/zlib.h for me, which is coming from zlib-devel package.

@karlkleinpaste
Copy link
Contributor

by this evening, i need either a tested fix or i'll hit it with reset --hard HEAD~ and push --force.

@greg-hellings
Copy link
Contributor

Working on it.

Out of curiosity... what does libgsf/minizip actually DO for Xiphos?

@yetist
Copy link
Contributor Author

yetist commented Apr 17, 2020

I forgot to update travis.yml, but I don't understand why Travis CI didn't report build status.

@karlkleinpaste
Copy link
Contributor

module archival, for single file move to other places, or...just for archives.
see mod.mgr, maintenance page, buttons at the bottom.

@greg-hellings
Copy link
Contributor

I just submitted #1004 that fixes the build errors (in my system).

However, when I archive KJV I get a 4.0KB zip file and a handful of GLib-CRITICAL messages in the console of the form:

(xiphos:885992): GLib-CRITICAL **: 12:50:28.057: g_dir_read_name: assertion 'dir != NULL' failed

(xiphos:885992): GLib-CRITICAL **: 12:50:28.057: g_dir_close: assertion 'dir != NULL' failed

I don't have time this second to dive into the functionality deficiencies, but the build error is, at least, fixed if someone else wants to revisit the functional testing.

@karlkleinpaste
Copy link
Contributor

i appreciate the fix pull, but at this point id rather see @yetist deal with it directly, both build and functionally, i.e., do some test archivals via mod.mgr and verify the results make sense.

@yetist
Copy link
Contributor Author

yetist commented Apr 17, 2020

I use Archlinux, build and create zip file is fine.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

@greg-hellings I am also able to build this with no issue, and it generates valid zip files. I also don't see the same Glib error you are seeing. I do get two messages when I hit the "Archive" button when the confirmation dialog pops up asking "Archive these modules?". The Yes/No buttons seem to work, but drawing the confirmation modal gives these on the console:

(xiphos:1294760): GLib-GObject-WARNING **: 22:00:15.602: invalid cast from 'GtkBox' to 'GtkButtonBox'

(xiphos:1294760): Gtk-CRITICAL **: 22:00:15.603: gtk_button_box_set_layout: assertion 'GTK_IS_BUTTON_BOX (widget)' failed

@greg-hellings
Copy link
Contributor

@alerque I also see those warnings, but I definitely got the two others. And I need to directly include the zlib.h header directly in utilities.c in order to build successfully.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

@yetist Arch Linux systems pretty much all have zlib by default. Are you sure you didn't reference something there by mistake that would make it a dependency, but not show up when building on Arch?

@karlkleinpaste
Copy link
Contributor

all right... i've had a busy day at work that's finally winding down. where are we at this point? @yetist says he builds fine without changes on arch. @alerque confirms this. F31 chokes on it in my case, as well as whatever @greg-hellings is using (i assume F-something-recent). shall i merge @greg-hellings' 1004 PR to fix this whole thing?

@greg-hellings
Copy link
Contributor

Worth noting:

Fedora 31 has minizip 2.9.1, which both Karl and I are on.
Arch ships with minizip 1.2.11.

The /usr/include/minizip/zip.h header on Arch has a #include <zlib.h> line in it, while the version on Fedora from a much later version of minizip does not.

It seems the discrepancy might be due to Arch having an impossibly ancient version of minizip by default. I wonder if that also accounts for the differences in the ZIP archives and warnings we're seeing.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

@greg-hellings Good find. If the zlib website which is the original home for minizip is to be believed, 1.2.11 is the very latest release. Alpine Linux, Debian, Ubuntu, Slackware, FreeBSD, etc. all package this version. It looks like Fedora and CentOS are packaging this fork instead.

@karlkleinpaste
Copy link
Contributor

1.2.11 is the very latest, and 3+ yrs old? really?
and i thought xiphos having gone 2 yrs was positively petrified.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

It looks like Fedora and CentOS have both the original and forks available. The Travis dependencies need to specify they want the 1.2 package: minizip1.2. There is probably something to do to fix the include statement as well but I'm not sure what that is.

@greg-hellings
Copy link
Contributor

Looks like the comparable package in Fedoraland is minizip-compat. The two are incompatible with one another. I can build without modification using minizip-compat, but the archive still fails to create.

@greg-hellings
Copy link
Contributor

Still seeing

(xiphos:894861): GLib-CRITICAL **: 14:51:11.799: g_dir_read_name: assertion 'dir != NULL' failed

(xiphos:894861): GLib-CRITICAL **: 14:51:11.799: g_dir_close: assertion 'dir != NULL' failed

@karlkleinpaste
Copy link
Contributor

i am uncomfortable building against packages named -compat because they have a tendency to disappear entirely in the N+1st release.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

@greg-hellings Are you sure the include detected and used libminizip.so.1 in preference to 2? You might need a CMake adjustment to request the so.1 version somehow.

@greg-hellings
Copy link
Contributor

Correction: the archive creates but only includes the .conf file. No data files. Is that correct?

@greg-hellings
Copy link
Contributor

@alerque

$ ldd /usr/local/bin/xiphos | grep zip
	libminizip.so.1 => /lib64/libminizip.so.1 (0x00007f6afe5f2000)

I completely removed and uninstalled minizip in favor of minizip-compat

@karlkleinpaste
Copy link
Contributor

no, of course not. the point of archival is to create a single file you can stow away or send to some other system.

@alerque
Copy link
Contributor

alerque commented Apr 17, 2020

@greg-hellings I get data files too.

@greg-hellings
Copy link
Contributor

Oh man. This seems to be something wonky on my system. Somewhere I ended up with kjv.conf installed but I don't actually have the module data files. So this seems to be behaving in exactly the way we would expect.

@karlkleinpaste
Copy link
Contributor

then if @greg-hellings has it actually working, using your updates in 1004, then i'll merge and we can declare victory and go home.

@greg-hellings
Copy link
Contributor

Victory has been declared! Re-installing the module with mod.mgr and running the archive has cleared both the console warnings and resulted in a properly built ZIP archive. And that's using the native minizip in Fedora.

Still needs the direct inclusion of zlib.h in Xiphos, though, as done in #1004

@yetist
Copy link
Contributor Author

yetist commented Apr 18, 2020

I think someone should check why travis did not report the build status here, so we can know the build status on other distro.

@greg-hellings
Copy link
Contributor

I'm beginning to wonder if it has something to do with Travis-ci.org versus .com. They've been slowly migrating from the org to the com for a long time. But I've recently seen them moving more rapidly.

I'll experiment with that in the future if we don't decide to move to Git Hub Actions entirely.

jtojnar added a commit to jtojnar/nixpkgs that referenced this pull request Jan 15, 2022
- Moved build dependencies to native.
- Most of those added in 4.2.1 update are only transitive.
- Switched to GTK 3.
- The GNOME 2 dependencies can me removed as they are not used.
  - Except for GTKHtml since WebKit2-based editor is not implemented.
- Python is not needed either.
- D-Bus functionality requires a patch to build
- gsf was replaced by minizip – crosswire/xiphos#1003
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

4 participants