GNOME Bugzilla – Bug 770380
Autoar master breaks build
Last modified: 2016-09-09 06:16:19 UTC
Autoar is being rewritten on the master branch, which causes the Evolution jhbuild to break. In particular, AutoarCreate no longer exists as such. See e.g. bug 768645.
Created attachment 334124 [details] [review] 0001-Match-changes-to-autoar-.pc-file-names.patch
Created attachment 334125 [details] [review] 0002-Match-changes-to-autoar-include-file-names.patch
Thanks for the bug report and patches, but: a) I prefer one patch for an issue, not tens of one-liner patches b) these changes are not enough, the build fails on the current git master of gnome-autoar, due to larger API changes I'm adding Carlos into the CC, he promised to provide more up-to-date patch for the evolution shortly. From the top of the build error with gnome-autoar git master at commit_abeed5c57ca, modulo bug #770376: > In file included from /usr/local/include/gnome-autoar-0/gnome-autoar/gnome-autoar.h:26:0, > from e-attachment-store.c:32: > /usr/local/include/gnome-autoar-0/gnome-autoar/autoar-compressor.h:51:7: > warning: redundant redeclaration of ‘autoar_compressor_get_type’ [-Wredundant-decls] > GType autoar_compressor_get_type (void) G_GNUC_CONST; > ^~~~~~~~~~~~~~~~~~~~~~~~~~ > In file included from /usr/local/include/gnome-autoar-0/gnome-autoar/gnome-autoar.h:26:0, > from e-attachment-store.c:32: > /usr/local/include/gnome-autoar-0/gnome-autoar/autoar-compressor.h:38:7: note: previous declaration of ‘autoar_compressor_get_type’ was here > G_DECLARE_FINAL_TYPE (AutoarCompressor, autoar_compressor, AUTOAR, COMPRESSOR, GObject) > ^~~~~~~~~~~~~~~~~~~~~~~~~~
Created attachment 334223 [details] [review] Fix archives support in attachments The API of gnome-autoar was recently modified so compression and extraction were no longer working. Replace autoar preferences objects with two shell settings. Save memory file to disk before extracting it. Handle generation of unique file names internally.
(In reply to Razvan Chitu from comment #4) > Created attachment 334223 [details] [review] [review] > Fix archives support in attachments > > The API of gnome-autoar was recently modified so compression and extraction > were no longer working. Replace autoar preferences objects with two shell > settings. AutoarPref was made to keep settings like format and filter shared between applications, so users can treat archive support as an integrated part of the desktop. They should be able to change the default setting with tools like gnome-control-center and expect it to work in all GNOME applications. This is particularly important for complex setting like 'pattern-to-ignore'. I know you removed the pattern checking feature, but you don't explain why filtering is not the responsibility of AutoarExtract. It is pretty annoying to manually remove useless files like '__MACOSX' and '.DS_Store' after extraction. > Save memory file to disk before extracting it. autoar_extract_new_memory_file was explicitly made to support evolution. I don't know why the feature is removed because the commit message doesn't explain it. I think the feature should be restored in autoar instead of making more temporary files. > Handle generation of unique file names internally.
(In reply to Ting-Wei Lan from comment #5) > (In reply to Razvan Chitu from comment #4) Hey! Thanks for taking the time to review my patch. > > Created attachment 334223 [details] [review] [review] [review] > > Fix archives support in attachments > > > > The API of gnome-autoar was recently modified so compression and extraction > > were no longer working. Replace autoar preferences objects with two shell > > settings. > > AutoarPref was made to keep settings like format and filter shared between > applications, so users can treat archive support as an integrated part of > the desktop. They should be able to change the default setting with tools > like gnome-control-center and expect it to work in all GNOME applications. > This is particularly important for complex setting like 'pattern-to-ignore'. The idea of sharing settings between applications does indeed sound good, but it has a few issues. A good example are the default formats and filters. If we want this to work, we need all the applications using autoar to display the same compression options. So we would probably have to agree on a shared UI first, which could be difficult. I'm saying this while thinking about the situation we had in Nautilus, where designers decided that a combobox displays too many options. > I know you removed the pattern checking feature, but you don't explain why > filtering is not the responsibility of AutoarExtract. It is pretty annoying > to manually remove useless files like '__MACOSX' and '.DS_Store' after > extraction. We thought that it would be better for AutoarExtract to just extract the content. By the way, do you have other use cases for this setting besides files added by OSX? > > > Save memory file to disk before extracting it. > > autoar_extract_new_memory_file was explicitly made to support evolution. I > don't know why the feature is removed because the commit message doesn't > explain it. I think the feature should be restored in autoar instead of > making more temporary files. Oh, I now feel bad because of not asking you about it before. However, my reason for removing it is that AutoarExtract had the combined API of what should be multiple classes. Keep in mind that the solution in evolution is temporary. I actually plan to restore that functionality by adding something like an AutoarStreamExtractor, which would work with a generic input stream. On top of that I'll re-implement the current Extractor, which will probably be called FileExtractor. What do you think? > > Handle generation of unique file names internally. Again, I don't think it should be the responsibility of autoar to automatically handle file conflicts. If autoar does (1) (2) (3) somebody might come and say that they want .00 .01 .02, right?
Review of attachment 334223 [details] [review]: Thanks for the patch. I have only the things below. Otherwise looks good, doesn't produce any compiler warnings with or without the gnome-autoar being compiled. I cannot speak about this things brought up by Ting-Wei, I was told that the things will change in the not so distant future. ::: e-util/e-attachment.c @@ +2120,3 @@ + + if (!e_enum_from_string (AUTOAR_TYPE_FORMAT, format_string, + (gint *)&format)) { This is not great, if the enum is not integer-sized, then it'll cause trouble. Better to have either a helper function for this, or define format and filter as gint, similarly as in e-attachment-store.c. @@ +2843,3 @@ + if (save_context->temporary_file != NULL) + g_object_unref (save_context->temporary_file); g_clear_object (&save_context->temporary_file); @@ +2904,3 @@ +static gchar * +get_new_name_with_count (const gchar *initial_name, + int count) gint @@ +3055,3 @@ + GFile *destination_directory; + GFile *new_destination; + int count = 0; gint
Created attachment 334233 [details] [review] Fix archives support in attachments The API of gnome-autoar was recently modified so compression and extraction were no longer working. Replace autoar preferences objects with two shell settings. Save memory file to disk before extracting it. Handle generation of unique file names internally.
(In reply to Razvan Chitu from comment #6) > (In reply to Ting-Wei Lan from comment #5) > > (In reply to Razvan Chitu from comment #4) > > Hey! Thanks for taking the time to review my patch. Hi! I haven't carefully read and tested your patch. The review I posted was mostly based on your commit message. > > > > Created attachment 334223 [details] [review] [review] [review] [review] > > > Fix archives support in attachments > > > > > > The API of gnome-autoar was recently modified so compression and extraction > > > were no longer working. Replace autoar preferences objects with two shell > > > settings. > > > > AutoarPref was made to keep settings like format and filter shared between > > applications, so users can treat archive support as an integrated part of > > the desktop. They should be able to change the default setting with tools > > like gnome-control-center and expect it to work in all GNOME applications. > > This is particularly important for complex setting like 'pattern-to-ignore'. > > The idea of sharing settings between applications does indeed sound good, > but it has a few issues. A good example are the default formats and filters. > If we want this to work, we need all the applications using autoar to > display the same compression options. So we would probably have to agree on > a shared UI first, which could be difficult. Although autoar-gtk was made to provide shared widgets, applications are not required to use it. autoar_gtk_chooser_*_new don't implicitly read default values from gsettings, Callers are required to pass arguments to provide the default. I think we only have to agree on the way to load and store settings. Applications are free to implement their custom widgets with default values retrieved from a shared location. > I'm saying this while thinking > about the situation we had in Nautilus, where designers decided that a > combobox displays too many options. I think we can add the widget made for nautilus to autoar-gtk-chooser if it doesn't depend on things specific to nautilus. > > > I know you removed the pattern checking feature, but you don't explain why > > filtering is not the responsibility of AutoarExtract. It is pretty annoying > > to manually remove useless files like '__MACOSX' and '.DS_Store' after > > extraction. > > We thought that it would be better for AutoarExtract to just extract the > content. By the way, do you have other use cases for this setting besides > files added by OSX? It is probably useful to ignore temporary files created by editors, such as .*.swp and .*.swo made by vim, and backup files with pattern *~. It is the creator of the archive that does the wrong thing, but this kind of mistake is not uncommon. Ignoring files added by Mac OS X is still the most important use case of this feature. Some archives include hundreds of them with different filenames, and I usually have to manually delete them with 'find <dir> -name <pattern> -delete' command because using nautilus to do it is too time-consuming. Removing this feature also prevents archives containing a top-level '__MACOSX' directory from being recognized as 'an archive with a top-level directory'. > > > > > > Save memory file to disk before extracting it. > > > > autoar_extract_new_memory_file was explicitly made to support evolution. I > > don't know why the feature is removed because the commit message doesn't > > explain it. I think the feature should be restored in autoar instead of > > making more temporary files. > > Oh, I now feel bad because of not asking you about it before. However, my > reason for removing it is that AutoarExtract had the combined API of what > should be multiple classes. OK, I agree that the memory input can be put in a different class as long as it doesn't result in code duplication. > Keep in mind that the solution in evolution is > temporary. I actually plan to restore that functionality by adding something > like an AutoarStreamExtractor, which would work with a generic input stream. > On top of that I'll re-implement the current Extractor, which will probably > be called FileExtractor. What do you think? Although libarchive calls itself a stream-oriented library, I remember there are a few format, such as 7-zip, doesn't work without a seek callback. For autoar to be fully functional, it still needs a 'seekable stream'. > > > > Handle generation of unique file names internally. > > Again, I don't think it should be the responsibility of autoar to > automatically handle file conflicts. If autoar does (1) (2) (3) somebody > might come and say that they want .00 .01 .02, right? Yes, I agree that the conflict resolution process can be interactive. The old behavior of adding (1) (2) (3) was copied from epiphany. I found you use query_exists in decide_destination_cb. I think it would be better to do make_directory to avoid race if possible. (I know the old code written by me also uses query_exists, so this is not a requirement but an improvement.)
Comment on attachment 334233 [details] [review] Fix archives support in attachments I'm setting this as 'reviewed' even I didn't make any real review. The problem is that this patch depends on a custom branch, not on a master or other official branch. I do not want the evolution to depend on such branches, thus I'm not committing this for the 3.22.0 (there will be releases, but this is really late in the release cycle). Let's wait for the 3.23.x development phase, maybe you'll also settle with the gnome-autoar changes as such, where it'll be also clear what changes will be required on the evolution side, or any other software side. By the way, the number added to the .pc files, I'd probably not do it unless really necessary, which it probably isn't here. The version number (and a soname version) is enough for API versionning, from my point of view.
I forgot to mention, 3.22.x evolution will be buildable against gnome-autoar at its commit_0300e4b31253779541a6f078ca45bd7a3bd6e7db.
*** Bug 770719 has been marked as a duplicate of this bug. ***
Created attachment 334663 [details] [review] Fix archives support in attachments The API of gnome-autoar was recently modified so compression and extraction were no longer working. Replace autoar preferences objects with two shell settings. Save memory file to disk before extracting it. Handle generation of unique file names internally.
Review of attachment 334663 [details] [review]: Thanks for the updated patch. It looks good. There are some plain 'char' types, which should be 'gchar' types (glib-char), but apart of this nitpick I didn't notice anything obvious. Feel free to commit this to the evolution sources. I didn't build-test it though, because my evolution cannot be currently compiled.
Attachment 334663 [details] pushed as b5681c5 - Fix archives support in attachments
Minor quibble: Should the minimum version not be bumped to 0.1.1?
Created attachment 335137 [details] [review] Bump minimum version of gnome-autoar to 0.1.1 Evolution depends on several features added in this version.
(In reply to Ting-Wei Lan from comment #17) > Created attachment 335137 [details] [review] [review] > Bump minimum version of gnome-autoar to 0.1.1 > > Evolution depends on several features added in this version. Except `pkg-config --modversion gnome-autoar-0` against gnome-autoar git master at its commit_4aec473d6f3 returns version 0.1.0, which breaks evolution build against it with the corrected (I will call it 'Correct', not 'Bump') version check. The released 0.1.1 returns proper version, thus I'm going to commit this, just make sure that you bump the git master version as necessary in the gnome-autoar the next time (and now). I hope this rush in the middle of the freezes will not repeat the next release cycle. (No offence meant, just expressing personal feelings.) Created commit c0155c6 in evo master (3.21.92+)
I updated the jhbuild moduleset to use gnome-3-22 branch of gnome-autoar: https://git.gnome.org/browse/jhbuild/commit/?id=3578943