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

Snap packaging #468

Closed
wants to merge 155 commits into from
Closed

Snap packaging #468

wants to merge 155 commits into from

Conversation

brlin-tw
Copy link
Contributor

Currently not buildable due to missing Boost dependency etc. , the packages from the PPA might help.

@GitCop
Copy link

GitCop commented Apr 23, 2018

Thanks for contributing! Unfortunately there were the following style issues with your Pull Request:

  • Commit: ece1a88
  • Your commit message body contains a line that is longer than 72 characters
  • Your subject line is longer than 50 characters

Please follow git commit message guidelines (you can use git push -f to update this PR).


This message was auto-generated by https://gitcop.com

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from a30a804 to 9e88cda Compare April 23, 2018 00:25
@GitCop
Copy link

GitCop commented Apr 23, 2018

Thanks for contributing! Unfortunately there were the following style issues with your Pull Request:

  • Commit: 06fc20a
  • Your commit message body contains a line that is longer than 72 characters

Please follow git commit message guidelines (you can use git push -f to update this PR).


This message was auto-generated by https://gitcop.com

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch 2 times, most recently from 61a98f5 to 30d2db6 Compare April 23, 2018 00:31
@vslavik
Copy link
Owner

vslavik commented Apr 23, 2018

Currently not buildable due to missing Boost dependency etc. , the packages from the PPA might help.

All dependencies are in the deps directory and it is preferable to build using them instead of some out-of-date PPAs. In particular, wxWidgets should absolutely be used from there instead of from Ubuntu's old packages.

I'm also a bit confused about why you submit something you yourself say is non-functional as a PR? The traditional way is to work on your git branch and submit a PR only when it is ready to be merged...

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Apr 23, 2018

All dependencies are in the deps directory and it is preferable to build using them instead of some
out-of-date PPAs.

I've successfully build the dependencies using the deps directory(though still barely launch-able), will push it tonight

I'm also a bit confused about why you submit something you yourself say is non-functional as a PR?
The traditional way is to work on your git branch and submit a PR only when it is ready to be merged...

I hope it to be noticed to avoid rebuilding wheels as I recently saw you mentioning it in another issue, anyway I'm fine with the tradition.

@brlin-tw brlin-tw closed this Apr 23, 2018
@vslavik
Copy link
Owner

vslavik commented Apr 23, 2018

will push it tonight

I'm even more confused: then why did you close this PR? You can use git push or git push --force to update this one...

@brlin-tw
Copy link
Contributor Author

@vslavik Just be a bit too coward, no worries

@brlin-tw brlin-tw reopened this Apr 23, 2018
@brlin-tw
Copy link
Contributor Author

brlin-tw commented Apr 23, 2018

I've prepared and pushed my current progress.

What's working:

  • Launch (with a few bypassable errors)
  • Load file
  • Save file

What's not:

  • (fixed) input methods
  • (gone) translation memory related errors
  • (not anymore) probably lots of stuff here and there
  • Crowdin integration(The auth webpage is triggered, but the result isn't sent back to the application)

Many problems probably related to incomplete desktop-gtk3 part integration, still looking at it.

@brlin-tw
Copy link
Contributor Author

I noticed that there's a template repo to track the progress:
https://github.com/snapcrafters/fork-and-rename-me

I wonder if it's sane to follow the snapcrafter's protocol instead?

@vslavik
Copy link
Owner

vslavik commented Apr 24, 2018

to follow the snapcrafter's protocol instead?

Meaning what, exactly?

@brlin-tw
Copy link
Contributor Author

Meaning what, exactly?

I mean follow the instructions from the snapcrafters template, I am like, jumped to the 20th task on the list or something.

@vslavik
Copy link
Owner

vslavik commented Apr 24, 2018

Just repeating what you said using different words doesn't explain anything I'm afraid. I am opposed to having it as separate repo if that's what you're asking. If it is not, I have no idea what you are asking :(

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from 96c5f2d to f785a8d Compare April 24, 2018 16:22
@brlin-tw
Copy link
Contributor Author

brlin-tw commented Apr 24, 2018

I am opposed to having it as separate repo if that's what you're asking.

That's the answer I'm looking for. Apologies for any annoyance.

I've added the missing integration of the desktop-gtk3 part, however, it now crashes on launch with the ibus integration somehow. Still looking at it.

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from 0c702f1 to 9e67ad3 Compare April 24, 2018 16:41
@vslavik
Copy link
Owner

vslavik commented Apr 28, 2018

switch dep. of wx to gtk2

That's not a good idea. Why?

Crowdin integration(The auth webpage is triggered, but the result isn't sent back to the application)

Probably some extra permission is required to pass the poedit:// protocol through? For normal deployment, this is defined in the poedit-uri.desktop file.

snappy: Improve boostlib build recipe …
snappy: Improve wxWidgets build recipe

Is there no way to build from the git submodule associated with this particular checkout?

Also, it makes sense to use static packages here...

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Apr 28, 2018

Thanks for reviewing.

switch dep. of wx to gtk2

That's not a good idea. Why?

The program crashes on launch at my end when using GTK3 for an unknown reason (related with input method framework, ibus) and at the time I thought that the checked out wxWidgets is GTK2 only, will drop it once the crash issue is sorted out.

For normal deployment, this is defined in the poedit-uri.desktop file.

I did notice the other desktop entry file, will try to integrate it when I find out how to do it properly.

Is there no way to build from the git submodule associated with this particular checkout?

It's definitely possible, looking into it.

Also, it makes sense to use static packages here...

I'm not completely sure how to achieve this, but will check it out.

@vslavik
Copy link
Owner

vslavik commented May 1, 2018

input methods

I think (based on docs) that this can be fixed by adding desktop-legacy to the snap's plugs list.

network entitlement is also needed for Crowdin.

translation memory related errors

Can you be more specific?

For normal deployment, this is defined in the poedit-uri.desktop file.

I did notice the other desktop entry file, will try to integrate it when I find out how to do it properly.

This thread suggests to have a "fake" app for this purpose in the apps list, associated with the other desktop file. If that's not feasible, then I can also fix it by merging both desktop files into one (this requires some extra changes to the code that I'd prefer to avoid, but is probably doable).

@brlin-tw
Copy link
Contributor Author

brlin-tw commented May 7, 2018

Apologies for the ignorance.

I think (based on docs) that this can be fixed by adding desktop-legacy to the snap's plugs list.

I did notice the doc but didn't trust it due to the lacking mention in snapcraft define desktop-gtk3, I've asked on the forum and got a confirmative answer today, will add it as soon as possible.

network entitlement is also needed for Crowdin.

Will do.

translation memory related errors

Can you be more specific?

I'll note it down once I encounter it again.

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from a6299b3 to bd7a77f Compare May 10, 2018 06:55
@GitCop
Copy link

GitCop commented May 10, 2018

Thanks for contributing! Unfortunately there were the following style issues with your Pull Request:

  • Commit: d5b70fe

  • Your subject line is longer than 50 characters

  • Commit: 8ee4eee

  • Your commit message body contains a line that is longer than 72 characters

  • Your subject line is longer than 50 characters

  • Commit: 29c4bad

  • Your subject line is longer than 50 characters

  • Commit: 61bdefe

  • Your commit message body contains a line that is longer than 72 characters

  • Your subject line is longer than 50 characters

  • Commit: bd7a77f

  • Your subject line is longer than 50 characters

Please follow git commit message guidelines (you can use git push -f to update this PR).


This message was auto-generated by https://gitcop.com

@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from bd7a77f to 8fd6906 Compare May 10, 2018 07:05
@brlin-tw
Copy link
Contributor Author

brlin-tw commented May 10, 2018

Known issues

  • (fixed)./src/common/iconbndl.cpp(359): assert ""icon.IsOk()"" failed in AddIcon(): invalid icon
    screenshot_20180510_151329

  • (fixed)wxWidgets stacktrace not viewable in the aforementioned error dialog, with the following message in terminal:

      sh: 1: addr2line: not found
      Debug: cannot read address information for stack frame #0
    
  • (fixed)Poedit crashes during launching : GLib-GObject-WARNING **: specified class size for type 'IBusIMContext' is smaller than the parent type's 'GtkIMContext' class size
    console output: https://pastebin.com/WpjftNy7

*.snap

## Ignore stages autogeneraged files
/*/parts/
Copy link
Owner

Choose a reason for hiding this comment

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

.gitignore in a subdirectory should use relative, not absolute (starting with /) paths to ignorable files...

Copy link
Contributor Author

@brlin-tw brlin-tw May 10, 2018

Choose a reason for hiding this comment

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

Refer to gitignore(5):

Patterns read from a .gitignore file in the same directory as the path, or in any parent directory, with patterns in the higher level files (up to the toplevel of the work tree) being overridden by those in lower level files down to the directory containing the file. These patterns match relative to the location of the .gitignore file. A project normally includes such .gitignore files in its repository, containing patterns for files generated as part of the project build.

I also have confirmed that the ignore rules are effective.

override-build: |
./configure
make -j$(nproc)
sudo make install
Copy link
Owner

Choose a reason for hiding this comment

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

There's no reason build should use sudo, I'm pretty sure.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This will be fixed in the next push.

@vslavik
Copy link
Owner

vslavik commented May 10, 2018

./src/common/iconbndl.cpp(359): assert ""icon.IsOk()"" failed in AddIcon(): invalid icon

That would indicate either misinstallation (some icon files omitted) or Poedit being unaware of its install location. That can be fixed by setting POEDIT_PREFIX, but it's probably better to figure out where in the snap things went wrong.

wxWidgets stacktrace not viewable in the aforementioned error dialog, with the following message

Including the binary in the snap would be one fix, but I think this is OK to ignore. I'll just disable this feature, it's not terribly useful anyway.

oedit crashes during launching : GLib-GObject-WARNING **: specified class size for type 'IBusIMContext' is smaller than the parent type's 'GtkIMContext' class size

That's pretty clear sign of miscompilation: different, binary-incompatible, version of GTK+ used to compile wxGTK and Poedit, and/or a runtime incompatible binary (which shouldn't normally be possible to link, but well).

I'll try to have a look at those issues myself if you don't beat me to it.

I also don't understand why you have two snaps there — instead of having one for the last release and one for git master, there should be only one that works with the current checked out tree, i.e. ., i.e. doesn't download anything from anywhere and builds from the sources around it.

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 25, 2018

I'll probably drop the entire ccache thing in the next push, the whole point of using ccache is to speed up development of the packaging(part rebuild time), as the implementation is close to complete it is no longer helpful but complicate things.

This patch patches the Autoconf files so that the poedit.1 manpage
won't be built during building snap.  Manpages in a snap are not
(yet) accessible from regular snap user[1] and the Asciidoc build
dependency will pull great amount of texlive packages, making the
build a lot longer.

[1] https://forum.snapcraft.io/t/support-for-man-pages/2299
This avoids the lengthy clone from the remote.
Ccache is originally incorporated as a measure to speed up the rebuild
process(to determine whether the recipe works), as the packaging is nearly
completed it is no longer that useful but complicates the recipe.

This patch drops the ccache part and also changes the behavior of the
gcc-6 recipe to create the faked GCC commands symbolic links at stage
phase instead of build phase for MacOS filesystem compatibility.

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from 460eb3e to 34a211d Compare August 26, 2018 14:18
@@ -82,12 +82,11 @@ parts:
- patches
- wxwidgets

source: git://github.com/vslavik/poedit.git
source: .git
Copy link
Owner

Choose a reason for hiding this comment

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

I don't understand this at all. Why not do source: . and build from the checkout?

Copy link
Contributor Author

@brlin-tw brlin-tw Aug 27, 2018

Choose a reason for hiding this comment

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

The source-type: local (when specifying source as .) copies all non-snapcraft-specific(/snap, /parts, /stage, and /prime) files and folders to the part's src folder (/parts/_part_name_/src), including the entire /.git folder and ignored files of the VCS.

When using source-type: git snapcraft only clones and check out the repository specified by source keyword, which is considered cleaner than source: ..

Copy link
Owner

Choose a reason for hiding this comment

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

It also prevents use of local modifications (bad for debugging) and most importantly, fetches a tree that is unrelated to the current snapcraft file, because it fetches the master. This means non-reproducible builds from unknown revision (same problem with the enchant part currently). That's An Enormously Bad Thing ™ — I already changed this to use reproducible builds locally.

It's rather bizarre that one cannot move the many snapcraft build directories to some location outside of the tree...

Copy link
Contributor Author

Choose a reason for hiding this comment

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

That makes sense, thanks for the explanation! Note there are source-commit and source-checksum keywords to have further control over the checked out part sources.

@vslavik
Copy link
Owner

vslavik commented Aug 26, 2018

I'll probably drop the entire ccache thing … the whole point of using ccache is to speed up development of the packaging

Given the transient nature of builds in containers, it never made much sense, but there are more issues with symlinks. Apparently, it's a Docker issue.

in the next push

I rebased on master and am no longer compatible with your branch.

@vslavik
Copy link
Owner

vslavik commented Aug 27, 2018

Do you by any chance understand why some child window (file open, about window) don't have any shadows in the snap, while some (preferences) and the main window do?

parallels picture

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 27, 2018

I have no idea about that, here's a comparison at my end:

default
(Ubuntu 18.04, XFCE, elementary Xfce dark theme)

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 27, 2018

Here's a screenshot produced in GNOME desktop environment:

2018-08-27 22-49-42

Is the about and file open dialog native Gtk dialogs? Smells like bug in the snapcraft-desktop-helpers's gtk3 support.

@brlin-tw
Copy link
Contributor Author

I've fowarded the issue to the seemingly responsible project:
[Gtk3] Window decoration defects with native Gtk dialogs · Issue #150 · ubuntu/snapcraft-desktop-helpers

@vslavik
Copy link
Owner

vslavik commented Aug 27, 2018

Is the about and file open dialog native Gtk dialogs?

Yes, exactly.

Smells like bug in the snapcraft-desktop-helpers's gtk3 support.

:(

@vslavik
Copy link
Owner

vslavik commented Aug 27, 2018

I have an update on the Crowdin links: Snapcraft pretty much ignores the command line and does not expand %u in the correct place. I'm writing a fix for it, but I think it's only part of the problem — even if the code gets through, it doesn't remember the auth token. It looks like libsecret is failing (and because Poedit's error reporting sucks, there's no visible information about it). This may be related to this error I get on every launch (i.e. before accessing libsecret and yes, I connected the snap per the instructions):

Failed to connect to session manager: None of the authentication protocols specified are supported

Can't understand it yet, it seems everybody is getting this error and blaming pretty much anything on it, with typical shotgun debugging advice on how to address. Do you get the error on launch too?

@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 27, 2018

Do you get the error on launch too?

Pretty much no, I might have seen this in other snaps but not this one.

@brlin-tw
Copy link
Contributor Author

I just gave you write access to my forking repo, you might want to force push your progress here (if you're into it, of course).

@vslavik
Copy link
Owner

vslavik commented Aug 28, 2018

Failed to connect to session manager: None of the authentication protocols specified are supported

Reading through wxGTK sources, this is due to a failed SmcOpenConnection() call, which is only used to detect whether we're running under GNOME/KDE, so it seems pretty unimportant.

vslavik pushed a commit that referenced this pull request Aug 28, 2018
Add support for building binary snap packages with snapcraft.

Snaps are containerised software packages that are simple to create and
install. They auto-update and are safe to run. And because they bundle
their dependencies, they work on all major Linux systems without modif-
ication.  -- from Snapcraft website

Refer-to: Snapcraft - Snaps are universal Linux packages
          <https://snapcraft.io/>

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@vslavik
Copy link
Owner

vslavik commented Aug 28, 2018

I just gave you write access to my forking repo, you might want to force push your progress here (if you're into it, of course).

By default PR branches can be pushed by repo owner already, but I instead pushed it to a branch in this repo: https://github.com/vslavik/poedit/tree/linux-snap

I also published it to the store (http://snapcraft.io/poedit), meaning you can safely remove poedit-wip-pr-468 from it.

If you don't see anything wrong with the snap, I'll merge it. Right now I'm aware of two small issues: 1. only for amd64 (I'll address that) and 2. in Software Center, the package is showing as "poedit" even though the metadata name is "Poedit 2" and even though your snap, as well as anything else in the store, is using the correct name... 🤷‍♂️

vslavik pushed a commit that referenced this pull request Aug 28, 2018
Add support for building binary snap packages with snapcraft.

Snaps are containerised software packages that are simple to create and
install. They auto-update and are safe to run. And because they bundle
their dependencies, they work on all major Linux systems without modif-
ication.  -- from Snapcraft website

Refer-to: Snapcraft - Snaps are universal Linux packages
          <https://snapcraft.io/>

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 29, 2018

meaning you can safely remove poedit-wip-pr-468 from it.

I'll unpublish that snap shortly after the last obsoletion notice update.

If you don't see anything wrong with the snap, I'll merge it.

I've installed the snap and tested a bit, no (new) issues noticed so far. Go ahead if you wish.

  1. in Software Center, the package is showing as "poedit" even though the metadata name is "Poedit 2" and even though your snap, as well as anything else in the store, is using the correct name...

Mine as well (KDE Discover 5.11.0, KDE neon):

screenshot_20180829_134134

I doubt the current snap store backend support fetching the store metadata...

Also note: Snap title and icon customizations seemed to be overwritten by newly published snap · Issue #746 · canonical-websites/snapcraft.io

林博仁 and others added 2 commits August 29, 2018 21:34
Signed-off-by: 林博仁(Buo-ren, Lin) <Buo.Ren.Lin@gmail.com>
Configure process errored, assuming that the config.* files are
outdated.

Signed-off-by: 林博仁(Buo-ren, Lin) <Buo.Ren.Lin@gmail.com>
@brlin-tw brlin-tw force-pushed the wip-snappy-packaging branch from 7a95587 to 3a3afc4 Compare August 29, 2018 13:35
vslavik pushed a commit that referenced this pull request Aug 30, 2018
Add support for building binary snap packages with snapcraft.

Snaps are containerised software packages that are simple to create and
install. They auto-update and are safe to run. And because they bundle
their dependencies, they work on all major Linux systems without modif-
ication.  -- from Snapcraft website

Refer-to: Snapcraft - Snaps are universal Linux packages
          <https://snapcraft.io/>

Signed-off-by: 林博仁(Buo-ren Lin) <Buo.Ren.Lin@gmail.com>
@vslavik
Copy link
Owner

vslavik commented Aug 30, 2018

I'll unpublish that snap shortly after the last obsoletion notice update.

Thanks!

I doubt the current snap store backend support fetching the store metadata…

It has to get them from somewhere and it's not the full huge snap package… I think this is just a caching issue, things went back to normal after I logged off and in again.

Also note: Snap title and icon customizations seemed to be overwritten by newly published snap

FWIW, I'm not seeing that.


I have now merged the snap support in 7b902ba — the content of this PR being squashed into 2d80574. Thanks a lot for your help with this!

@vslavik vslavik closed this Aug 30, 2018
@brlin-tw
Copy link
Contributor Author

brlin-tw commented Aug 31, 2018

Thanks a lot for your help with this!

It's my pleasure.

You might want to:

@vslavik
Copy link
Owner

vslavik commented Aug 31, 2018

Yeah, I should… I'm giving it a bit of time for beta testing by more users, to see if there are any issues, and will update information about it a bit later (~a week or so I think, depending on how it goes).

@brlin-tw brlin-tw deleted the wip-snappy-packaging branch November 22, 2018 11:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants