search

shutter-project / shutter

Screenshot tool for Linux
https://shutter-project.org/
GNU General Public License v3.0
503 stars 34 forks source link

Shutter 0.99.3 throws segmentation fault on Ubuntu 22.04+ #561

Closed jhonny-oliveira closed 1 year ago

jhonny-oliveira commented 1 year ago

Brief summary of issue

I built shutter for Ubuntu 22.04+. However, I get "Segmentation fault (core dumped)" when running it.

Steps to reproduce the issue

run shutter

Extra information

Same problem on both display servers: Xorg or Wayland.

Build and execution/error logs: shutter_0.99.3_build_log.txt shutter_0.99.3_error_log.txt

Any ideas?

Photon89 commented 1 year ago

Hi, thanks for packaging! Are there some patches being applied? In line 7174 in bin/shutter there is no remove_submenu method opposed to what the error output states:

Can't locate object method "remove_submenu" via package "Gtk3::MenuItem" at /usr/bin/shutter line 7174, <DATA> line 19.
    Shutter::App::fct_update_profile_selectors(Gtk3::ComboBoxText=HASH(0x55d821978b88), ARRAY(0x55d81f58ede0)) called at /usr/bin/shutter line 2687
    Shutter::App::STARTUP(Shutter::App=HASH(0x55d820d58120)) called at /usr/lib/x86_64-linux-gnu/perl5/5.34/Glib/Object/Introspection.pm line 67
    Glib::Object::Introspection::__ANON__(Shutter::App=HASH(0x55d820d58120)) called at /usr/bin/shutter line 10922
Segmentation fault (core dumped)
DarthGandalf commented 1 year ago

@Photon89 you may be looking at a wrong version, https://github.com/shutter-project/shutter/blob/v0.99.3/bin/shutter#L7174 matches the description.

This is one more leftover from Gtk3 upgrade: https://developer-old.gnome.org/gtk2/stable/GtkMenuItem.html#gtk-menu-item-remove-submenu was deprecated in Gtk2, and never existed in Gtk3.

@jhonny-oliveira can you try #562 ?

DarthGandalf commented 1 year ago

@Photon89 by the place where it crashes, it looks like it was caused by #489 in the case if there are no profiles yet - the old (broken) setups already had a profile named * due to the bug you fixed, therefore it always took the branch where there's a submenu to show.

jhonny-oliveira commented 1 year ago

Thank you both! The problem is solved.

Photon89 commented 1 year ago

Right, I forgot that there was #555 after release, sorry for the confusion!

jhonny-oliveira commented 1 year ago

I'm a bit surprised that nobody else reported this. According to https://repology.org/project/shutter/versions this version has been already published in quite a few places. Not sure if you guys can easily reach out to the maintainers and avoid the end user drama.

My packages are available in the usual place: https://xtradeb.net/apps/shutter

On a separate thread, I noticed you made several changes that simplified the packaging. Nevertheless, I'm still applying the following tweaks:

rules:

#!/usr/bin/make -f

%:
    dh $@

execute_after_dh_install override_dh_auto_install: PACKAGE := $(firstword $(shell dh_listpackages))

override_dh_auto_install:
    dh_auto_install -- prefix=debian/$(PACKAGE)/usr

execute_after_dh_install:
    dh_install
    mkdir -p debian/$(PACKAGE)/usr/share/perl5/
    mv debian/$(PACKAGE)/usr/share/shutter/resources/modules/Shutter debian/$(PACKAGE)/usr/share/perl5/
    rm -fr debian/$(PACKAGE)/usr/share/shutter/resources/modules/ debian/$(PACKAGE)/usr/share/doc/*/COPYING
    mv debian/$(PACKAGE)/usr/share/appdata debian/$(PACKAGE)/usr/share/metainfo

Is this something worth looking into?

Thank you for the prompt response!

DarthGandalf commented 1 year ago

I'm a bit surprised that nobody else reported this.

This happens only for new users of Shutter. Old users (who already have a configuration at ~/.shutter) were affected by bug #453 which caused them to have at least 1 profile always. Since the bug was fixed, that profile is no longer created, but shutter doesn't automatically remove old profiles, so new users just don't get it created for them, exposing this bug.

I'm still applying the following tweaks Is this something worth looking into?

Probably. For the Gentoo package I'm also ignoring what our Makefile states in the install:, and copying all the files manually to the image dir. Apparently I'm not the only one, but since this is easily fixable on Gentoo side, I didn't want to break this Makefile for case if it works correctly for some other distro.

I understand the metainfo and COPYING changes. Why do you need to move modules into /usr/share/perl5/ ?

DarthGandalf commented 1 year ago

@Photon89 we'll have to make a new release with this fix

jhonny-oliveira commented 1 year ago

Dear @DarthGandalf ,

Why do you need to move modules into /usr/share/perl5/ ?

I hope my answer is not too disappointing. Upstream (Debian/Ubuntu) does it this way. My guess, is that this it the default system location to place perl modules.

Thank you!

DarthGandalf commented 1 year ago

This would make sense if there is a goal to make these modules usable from custom perl programs. But they are internal to shutter

пт, 10 мар. 2023 г., 11:31 Jhonny Oliveira @.***>:

Dear @DarthGandalf https://github.com/DarthGandalf ,

Why do you need to move modules into /usr/share/perl5/ ?

I hope my answer is not too disappointing. Upstream (Debian/Ubuntu) does it this way. My guess, is that this it the default system location to place perl modules.

Thank you!

— Reply to this email directly, view it on GitHub https://github.com/shutter-project/shutter/issues/561#issuecomment-1463672328, or unsubscribe https://github.com/notifications/unsubscribe-auth/AACPLZAPHQ27TS2Y7YDKMFLW3MGIPANCNFSM6AAAAAAVVQ2IM4 . You are receiving this because you were mentioned.Message ID: @.***>

jhonny-oliveira commented 1 year ago

I updated the build recipe inline with the previous comments and your latest updates. Thank you!

This is the relevant bit of recipe (debian/rules).

#!/usr/bin/make -f
# -*- makefile -*-

%:
    dh $@

execute_after_dh_install override_dh_auto_install: PACKAGE := $(firstword $(shell dh_listpackages))

override_dh_auto_install:
    dh_auto_install -- prefix=debian/$(PACKAGE)/usr

execute_after_dh_install:
    rm -fr debian/$(PACKAGE)/usr/share/shutter/resources/modules/X11 debian/$(PACKAGE)/usr/share/doc/*/COPYING

This, the relevant build log: build_log.txt .

I also tried to remove "override_dh_auto_install", but I noticed that the install goal does not respect the DESTDIR (see here build_non_overridden_auto_install_with_error.txt ) and the build fails.

If it is not asking too much, would it be possible to ensure the install goal deploys everything within DESTDIR (DESTDIR/prefix, etc...)? This way we could further simplify the recipe and avoid yet another override. This change should have absolutely no impact on the application itself, as it's only meant to capture all file in an desirable location for packaging purposes. The application paths (prefix, share, etc...), remain the same.

Photon89 commented 1 year ago

To be honest, It was the first Makefile I pieced together, any feedback is welcome! Shall it be $(DESTDIR)/$(prefix)/... instead of $(prefix)/...?

DarthGandalf commented 1 year ago

Please open a pull request

DarthGandalf commented 1 year ago

Btw, why are you removing /usr/share/shutter/resources/modules/X11 ?

jhonny-oliveira commented 1 year ago

Dear @Photon89 ,

I don't know the official answer, neither looked for it. But, I can confirm that from the couple of other packages I went through, that is the case. "DESTDIR" prefixes all install paths and is typically not set or DESTDIR=''.

Dear @DarthGandalf ,

same answer as before, upstream is doing it (https://salsa.debian.org/perl-team/modules/packages/shutter/-/blob/debian/0.99.2-4/debian/rules). They move Shutter and WebService modules to a system path and remove the rest (X11 included). My guess is that it is because X11 already exists in the Ubuntu system path. So, independently of moving the modules or not to the system path, at least in Ubuntu X11 should be removed, so the system one is used. Unless, you say otherwise. :-)

Photon89 commented 1 year ago

Damn, have we been bundling modules with Shutter which are available otherwise till now? @jhonny-oliveira Do you by chance know, which package provides these modules in Ubuntu/Debian?

DarthGandalf commented 1 year ago

Looks like https://packages.debian.org/sid/libx11-protocol-other-perl

DarthGandalf commented 1 year ago

https://metacpan.org/pod/X11::Protocol::Ext::XFIXES

Photon89 commented 1 year ago

Oh man... Shall we remove it and add this as dependency then?

DarthGandalf commented 1 year ago

yeah, I think so

jhonny-oliveira commented 1 year ago

Yes, upstream is using libx11-protocol-other-perl. I have also built this patch: Add_DESTDIR_to_Makefile.txt.

Now, debian/rules looks like this:

#!/usr/bin/make -f

%:
    dh $@

execute_after_dh_install: PACKAGE := $(firstword $(shell dh_listpackages))

override_dh_auto_install:
    dh_auto_install -- PREFIX=/usr

execute_after_dh_install:
    rm -fr debian/$(PACKAGE)/usr/share/shutter/resources/modules/X11 debian/$(PACKAGE)/usr/share/doc/*/COPYING

Let me know when you incorporate the changes discussed here (or equivalent), so I can update the package.

One last thing, the goal all, get's executed every time on make (all) and make install. Do you know how to make it be execute only when it's needed? Like, only on changes. This way, if we run make and later on make install (all) does not get executed the second time.

Thank you!

DarthGandalf commented 1 year ago

One last thing, the goal all, get's executed every time on make (all) and make install.

It's possible. But it'd need a more complicated Makefile which tracks generated files properly. Basically it means reimplementing po2mo.sh inside Makefile itself. I'm not up for that task. We could switch to CMake or something but that would still be more complicated code than what we have now. I don't think the benefit here outweighs the complexity needed.

DarthGandalf commented 1 year ago

The current Makefile doesn't even mark the targets it declares as phony, which would be the best practice.

What I mean is the current build system is horribly broken, but it kinda does the job, and is good enough for the purpose. You can try rewrite it to something more sane, if you wish to.