GNOME Bugzilla – Bug 734718
Update Autoconf version specific libparted checks and defines to feature specific ones
Last modified: 2014-10-20 17:10:25 UTC
This bug will cover a number of cleanups to the Autoconf checks in configure.ac while working on: bug #734076 - Autodetect parted online partition resizing capability Configure.ac has multiple checks for versions of libparted and the C++ code has conditional code depending HAVE_LIBPARTED_n_n_n_PLUS defines. Change these to feature specific checks where possible and feature specific definitions. It is the Autoconf way, easier to understand and more portable. Patchset to follow. Mike
Created attachment 283326 [details] [review] Autoconf check tidyups for libparted versions (v1) Hi Curtis, Here's the patchset for this. As described above it is mostly about not checking for version of libparted but checking for features and using suitable feature #define names instead. These changes are: HAVE_LIBPARTED_3_1_0_PLUS -> Autoconf checks for ped_file_system_resize() in libparted and libparted-fs-resize() and existing #define HAVE_LIBPARTED_FS_RESIZE HAVE_LIBPARTED_3_0_0_PLUS -> Removed. Just use internal GParted copy for fat16/32 always rather than libparted implementation with version <= 2.4. HAVE_LIBPARTED_2_2_0_PLUS -> Autoconf still checks for libparted version >= 2.2 but now #define ENABLE_PT_REREAD_WORKAROUND and #define USE_LIBPARTED_LARGE_SECTOR_SUPPORT as appropritately. It also passes the needed libparted and libparted-fs-resize libraries, after finding them in ./configure, via the LIBS variable into the Makefiles rather than hardcoding them in the Makefiles. Plus a couple of other small tidyups. Summary of testing: 1) Build on Fedora 20 with libparted 3.1. New resize library. Fat16/32 can be resized. grep LIBPARTED_FS_RESIZE config.h : Defined gpartedbin : Can resize fat16/32. PASS 2) Build on Fedora 14 with libparted 2.3. Old resize API available. Fat16/32 can be resized. grep LIBPARTED_FS_RESIZE config.h : Defined gpartedbin : Can resize fat16/32. PASS 3) Test build CentOS 5.10 with libparted 1.8.1. Get: grep PT_REREAD_WORKAROUND : Defined grep LARGE_SECTOR_SUPPORT config.h : Undefined Also debugged the code to ensure that code in ENABLE_PT_REREAD_WORKAROUND is used. PASS 4) Test build all revisions on multiple distributions. git-test-sequence origin/master.. testbuild.sh CentOS 5.10 with libparted 1.8.1 - PASS Fedora Rawhide with libparted 3.2 - PASS Debian 6 with libparted 2.3 - PASS CentOS 6.5 with libparted 2.1 - PASS Specifically don't have a distro with libparted 3.0 anywhere to confirm that HAVE_LIBPARTED_FS_RESIZE is not defined and therefore GParted can't resize fat16/32 file systems. Thanks, Mike
Hi Mike, Thanks for digging deeper into the autoconf and tidying up configure.ac. Changes that make the code more readable, and hence maintainable, are certainly appreciated. :-) Following are two comments. One is a short description of how I test with a specific parted version, and the second is a question on one of the patches. Using Specific Parted Version ----------------------------- 1) Download the parted version you wish to test (e.g., 3.0) from: http://ftp.gnu.org/gnu/parted/ 2) Extract parted tarball, configure, make, and make install. Note that by default this will install into /usr/local. 3) Set library environment variables to look in /usr/local/lib. $ export LD_LIBRARY_PATH=/usr/local/lib $ export LD_RUN_PATH=/usr/local/lib $ export LDFLAGS=-L/usr/local/lib 4) Build gparted. When you run autoconf or ./configure you should see the specific version of parted listed in the check for libparted >= 1.7.1. You will also see the parted version listed when running gparted from the command line. Question: What do you think of removing P4? ---------------------------------------------- "P4 - Remove little used HAVE_LIBPARTED_3_0_0_PLUS definition" changes all fat16 and fat32 moves to use gparted's internal copy algorithm. This is good for consistency; however, there is a significant performance hit when using parted versions < 3.0. This is because parted versions 2.4 and lower have intelligence about the location of files within fat16/32 file systems and hence do not move all of the unused space. For example I created an 8192 MiB FAT32 partition, mounted it, and then copied the gparted-live-0.19.1-1-i486.iso file to FAT32. This was so there was a file in the file system that I could check with md5sum before and after the move. Without files, the difference in move times is even greater. 0:18 - time for move with GParted 0.19.1 * 5:40 - time for move with this patch set applied, specifically P4. Note that the advantage will likely shrink for file systems that contain less unused space (more full :-). I used libparted-2.3 for this test, but the results should hold true for libparted <= 2.4. [*] The GParted GUI interface hung for a short while during this test. This might be an anomaly with my system so I'll keep an eye on it to see if it warrants further investigation. I will continue to test the patch set and will let you know if I have any further questions. Curtis
Hi Curtis, I've done some initial testing with a 10 GiB Fat32 partition, 33% full with a few large ISOs. Using GParted compiled with libparted 2.4 (hence using libparted to move the partition rather than GParted). With GParted 0.14.0 the UI is responsive while libparted takes ~5 minutes to move the partition. With GParted 0.15.0 the UI is totally unresponsive while for the 5 minutes! Looks like: Refactor to use asynchronous command execution (#685740) has made all the long running libparted calls run in the main UI thread. Assuming this is the case, keeping P4 and always using the GParted internal copy to perform the partition move is a smalls step to improving the situation. This still leaves file system resizing and any other long running libparted actions running in the main UI thread to cause it to hang. I'm surprised users haven't complained about this. Mike
Thanks Mike for confirming the GUI hang problem and narrowing it down to changes for GParted 0.15.0. From reviewing the changes between 0.14.1 and 0.15.0 I think the change in behaviour is due to the removal of Dialog_Progress::thread_apply_operation(). Reduce threading (#685740) https://git.gnome.org/browse/gparted/commit/?id=52a2a9b00a32996921ace055e71d0e09fb33c5fe Previously thread_apply_operation() would ensure that all apply operations would be threaded, regardless of whether the operation was an external command or internal code/library calls. I am contemplating what would be involved in re-introducing threading, even if only for libparted operations of fat16/32 & HFS/+ resizes and fat16/32 moves. I do like the improved progress tracking enabled by asynchronous external command execution. However I think we need to address the long running internal code/library calls. In 0.15.0, Phillip converted the copy algorithm to be threaded so we might consider something similar. Thread the internal copy algorithm (#685740) https://git.gnome.org/browse/gparted/commit/?id=bd9e16f22fe3dfae73064369aadeda5de10b61be I too am surprised that we haven't received complaints about GUI hangs. Now to figure out how to address this situation.... Curtis
Created attachment 283569 [details] [review] Compile fixes for commit f5a5c9c Hi Curtis, I've confirmed it by git bisecting the change in behaviour of libparted being called in the UI thread and hanging to the commit you mentioned: Reduce threading (#685740) https://git.gnome.org/browse/gparted/commit/?id=52a2a9b00a32996921ace055e71d0e09fb33c5fe It was a bit harder because the 4 previous commits to this don't compile! Attached 4 line patch needed to compile it. Getting back to the fate of P4/9 - Remove little used HAVE_LIBPARTED_3_0_0_PLUS definition (#734718). I only see 2 options at the moment: 1) Take the patch as it is; 2) Wait until we restore threading for libparted API calls. I think it might take a while to understand and implemented what we need so I prefer to take the patch as it is at the moment. When threading use of libparted is done we can then review which method we use to move FAT16/32 file systems and I see 3 options: 1) Stick with using GParted always; 2) Switch back to libparted for versions <= 2.4 and GParted otherwise (as now); 3) Use libparted whenever ped_file_system_resize() is found (HAVE_LIBPARTED_FS_RESIZE is defined) so only use GParted move with version 3.0, otherwise use libparted. I'd just like to get this patchset out of the way before moving on to another task. Thanks, Mike
Hi Mike, Your reasoning makes sense to me. I think we're better to proceed with this patch set, and then address the libparted threading later. For now I will ignore the libparted threading stuff and focus again on reviewing this patch set with an eye towards committing the patches. Curtis
Hi Mike, I just encountered a problem with "make distcheck" on kubuntu 12.04. The error message is as follows: <snip> ../../src/GParted_Core.cc: In member function ‘void GParted::GParted_Core::LP_set_used_sectors(GParted::Partition&, PedDisk*)’: ../../src/GParted_Core.cc:1819:53: error: ‘ped_file_system_open’ was not declared in this scope ../../src/GParted_Core.cc:1823:60: error: ‘ped_file_system_get_resize_constraint’ was not declared in this scope ../../src/GParted_Core.cc:1832:31: error: ‘ped_file_system_close’ was not declared in this scope ../../src/GParted_Core.cc: In member function ‘bool GParted::GParted_Core::resize_move_filesystem_using_libparted(const GParted::Partition&, const GParted::Partition&, GParted::OperationDetail&)’: ../../src/GParted_Core.cc:2368:39: error: ‘ped_file_system_open’ was not declared in this scope ../../src/GParted_Core.cc:2376:63: error: ‘ped_file_system_resize’ was not declared in this scope ../../src/GParted_Core.cc:2378:31: error: ‘ped_file_system_close’ was not declared in this scope make[3]: *** [GParted_Core.o] Error 1 make[3]: Leaving directory `/home/gedakc/workspace/gparted.dev2/gparted-0.19.1-git/_build/src' make[2]: *** [all-recursive] Error 1 make[2]: Leaving directory `/home/gedakc/workspace/gparted.dev2/gparted-0.19.1-git/_build' make[1]: *** [all] Error 2 make[1]: Leaving directory `/home/gedakc/workspace/gparted.dev2/gparted-0.19.1-git/_build' make: *** [distcheck] Error 1 I think the error arises due to the removal of the conditional inclusion of <parted/filesys.h> from GParted_Core.h in the third patch. Do you encounter the same problem? Curtis
Doh! <face-palm> My bad. I forgot to set the environment variables while testing with parted-3.1 on kubuntu 12.04. Please ignore comment #7. :-)
Hi Mike, Good work on this latest patch set. My testing of the patch set in comment #1 was successful, though I did find some unrelated problems with ubuntu 14.04 and the libparted package in the distro. Distro libparted Result Test -------------- --------- ------ -------------------------- opensuse 13.1 2.4 Pass Move and resize fat16 kubuntu 12.04 2.3 Pass Move and resize fat32 kubuntu 12.04 2.4 * Pass Move and resize fat32 kubuntu 12.04 3.0 * Pass Move fat32, cannot resize (expected) kubuntu 12.04 3.2 * Pass Move and resize fat32 ubuntu 10.04 2.2 Pass Move and resize fat16 ubuntu 14.04 2.3 FAIL Crash/hang due to distro libparted ** ubuntu 14.04 3.2 * Pass Move and resize fat32 * This libparted version installed from tarball and is not the version packaged with the GNU/Linux distribution. ** While testing fat16/fat32 resizes in ubuntu 14.04, I encountered multiple crashes and hangs with GParted when linked with distro package libparted-2.3-19ubuntu1. As soon as I upgraded to libparted 3.2, all these crashes disappeared. For example create a 512 MiB fat32 partition and apply. Next grow it to 532 MiB. This regularly crashes or hangs in ubuntu 14.04 when using the libparted distro package, but works correctly in kubuntu 12.04. This problem seems specific to ubuntu 14.04 and the libparted package in ubuntu 14.04. Note that these crashes exist even without the patch set for this bug report applied (e.g., 0.19.1 release version). Hence the patch set in comment #1 is not the cause of these crashes. Further researching the cause of these crashes is a potential exercise for another day (Phillip?) Note that "make distcheck" also passed in all of the above listed configurations. The patch set in comment #1 has been committed to the GNOME repository for inclusion in the next release of GParted. The relevant git commits can be viewed at the following links: Removed unused variable need_work_around from configure.ac (#734718) https://git.gnome.org/browse/gparted/commit/?id=2cd82dfc38d14b8d5f02b613a25b551fcfb84d8b Update AM_PROG_LIBTOOL to AC_PROG_LIBTOOL in configure.ac (#734718) https://git.gnome.org/browse/gparted/commit/?id=654cdc7335beda48f0da095a1d06e91870d9ac76 Use Autoconf check specifically for libparted-fs-resize (#734718) https://git.gnome.org/browse/gparted/commit/?id=c1db9811e1e7a7caa1582c724e63b8a3be01992e Remove little used HAVE_LIBPARTED_3_0_0_PLUS definition (#734718) https://git.gnome.org/browse/gparted/commit/?id=288c4dbf2e345c9e845b7b4e95120cda032a5fc6 Use specific Autoconf check for FS resize capability in libparted (#734718) https://git.gnome.org/browse/gparted/commit/?id=eb7b706f23312631c677ab8cb91accbaca13a45e Rename HAVE_LIBPARTED_2_2_0_PLUS define into feature names (#734718) https://git.gnome.org/browse/gparted/commit/?id=ed4ea6cf03d74184b5e91d23496e5bb7a4569468 Only check for ped_file_system_resize() once if possible (#734718) https://git.gnome.org/browse/gparted/commit/?id=080b3b080d48456180d84c04cdbc1766ccf9c96d Only check libparted version once and cache the result (#734718) https://git.gnome.org/browse/gparted/commit/?id=bb17f44e996c638fa435ccc66aa17ae968e9d5dd Pass link libraries via LIBS variable into Makefiles (#734718) https://git.gnome.org/browse/gparted/commit/?id=3030118caf1f101bafef31c4e60d15a9be21b9e3 My next step is to review: bug #734076 - Autodetect parted online partition resizing capability Curtis
This enhancement was included in the GParted 0.20.0 release on October 20, 2014.