bugGNU Octave - Bugs: bug #53695, tar.m and unpack.m are...

 
 

bug #53695: tar.m and unpack.m are incompatible with BSD tar

Submitter:  Mike Miller <mtmiller>
Submitted:  Wed 18 Apr 2018 10:49:19 PM UTC
   
 
Category:  Octave Function Severity:  3 - Normal
Priority:  5 - Normal Item Group:  Incorrect Result
Status:  Fixed Assigned to:  None
Originator Name:  Open/Closed:  * Closed
Release:  * dev Operating System:  * BSD
Fixed Release:  None Planned Release:  None
* Mandatory Fields

Add a New Comment Rich Markup
   

Jump to the original submission

Tue 13 Aug 2019 09:19:38 AM UTC, comment #29: 

Sounds good. Sorry about the incorrect code formatting.

Andrew Janke <apjanke>
Wed 07 Aug 2019 07:25:55 PM UTC, comment #28: 

I pushed the change on the default branch.

@apjanke - I edited the commit message to conform to Octave's coding standards. I also pushed a commit to include the new function file in the automake build system.

https://hg.savannah.gnu.org/hgweb/octave/rev/3bf19af20af8
https://hg.savannah.gnu.org/hgweb/octave/rev/c80ba7295b5e

Mike Miller <mtmiller>
Group Member
Tue 06 Aug 2019 07:20:59 PM UTC, comment #27: 

I have not reviewed tar-support-BSD-tar-v3.patch yet. I'll try to get to that today and push it on default.

Mike Miller <mtmiller>
Group Member
Tue 06 Aug 2019 03:43:03 PM UTC, comment #26: 

@Mike: You reviewed this patch for BSD tar in January, 2019.  Can it be committed now?

Rik <rik5>
Group administrator
Tue 22 Jan 2019 01:09:50 AM UTC, comment #25: 

Okay, one more patch: file #46067:  tar-support-BSD-tar-v3.patch

Andrew Janke <apjanke>
Tue 22 Jan 2019 12:11:06 AM UTC, comment #24: 

Right, the relevant testing on Windows is more to make sure that the tar_is_bsd function works correctly and no accidental regressions were introduced. I don't expect Windows users to be needing BSD tar support at all.

The latest patch revision looks better to me, just please remove the "#TAR_IS_BSD" doc comment line, and a more complete commit message.

There doesn't seem to be a consensus yet on whether this is suitable for stable, I don't have a particular preference.

Mike Miller <mtmiller>
Group Member
Mon 21 Jan 2019 11:50:42 PM UTC, comment #23: 

I kinda tested on Windows 10 using Octave 4.4.1 (using the Windows installer downloaded from https://www.gnu.org/software/octave/download.html) and these new scripts. Seems to work. "test tar" passes. "test unpack" fails, but I think that's because I'm mixing stable M-files with a 4.4.1 Octave.


>> test tar
PASSES 6 out of 6 tests
>> test unpack
>> test unpack
***** testif HAVE_ZLIB
 envvar = {"TMPDIR", "TMP"};
 envdir = cellfun (@(x) getenv (x), envvar, "uniformoutput", false);
 unwind_protect
   cellfun (@(x) unsetenv (x), envvar);
[...]
 end_unwind_protect
!!!!! test failed
'__octave_link_file_remove__' undefined near line 139 column 5


Are there something like nightly or snapshot builds of stable available for Windows? I'm not set up to build Octave on Windows.

I suspect this won't be a problem on Windows, because there, instead of using a tar program provided by the system, Octave ships with a GNU tar.exe in its own bin dir, and that bin dir is right near the front of the %PATH% in the Octave process, so it's unlikely to be shadowed by a tar command from another source. (And for that matter, I don't know of any source of a BSD tar on Windows; there's no BSD equivalent of WSL/Cygwin/msys AFAIK.)

Andrew Janke <apjanke>
Mon 21 Jan 2019 06:34:26 PM UTC, comment #22: 

Here's a revision addressing your concerns: file #46061:  tar-support-BSD-tar-v2.patch

I think I got all the trailing whitespace.

Andrew Janke <apjanke>
Mon 21 Jan 2019 06:20:37 PM UTC, comment #21: 

The patch works for me on Debian, tested with both 'tar' and 'bsdtar'.

A few style comments:

  • "if tar_is_bsd ()" should be "if (tar_is_bsd ())"
  • The tar_is_bsd.m function file needs a proper copyright, license, and Texinfo doc string
  • There is some trailing whitespace added in tar.m and tar_is_bsd.m, and tar_is_bsd.m is missing a newline at the end of the file
Mike Miller <mtmiller>
Group Member
Mon 21 Jan 2019 05:33:08 PM UTC, comment #20: 

I'll test on Windows.

> IIRC BSD tar sends some of its output to stderr which broke a few things here and there in Octave[...]


This patch is handling that by just redirecting BSD tar's stderr to stdout: "tar cvf %s -C %s %s 2>&1".

That could cause problems if actual error diagnostics were emitted on stderr during file creation, if there were diagnostics but the file creation succeeded and returned 0. Not sure what cases those would be, or how they could be handled, though.

Andrew Janke <apjanke>
Mon 21 Jan 2019 05:25:42 PM UTC, comment #19: 

I'd also argue this is a bug fix: as is, the tests for tar fail, and "f = tar()" and "f = untar()" produce incorrect results on systems with BSD tar installed as the default.

Andrew Janke <apjanke>
Mon 21 Jan 2019 05:17:41 PM UTC, comment #18: 

One could also argue that this is bug fix: Octave does not check which tar it is actually using and "GNU tar" is (as far as I know) not an official dependency.

Don't get me wrong, I rather prefer to have this patch in the development branch than nowhere :)
 
Sebastian

Sebastian <sschoeps>
Mon 21 Jan 2019 05:04:38 PM UTC, comment #17: 

This is a feature improvement so the eventual patch should go on to the development branch.

Rik <rik5>
Group administrator
Mon 21 Jan 2019 07:45:00 AM UTC, comment #16: 

Given the issues touched in bug #44007 I think this patch also needs verifying in Windows / mxe-octave.

IIRC BSD tar sends some of its output to stderr which broke a few things here and there in Octave, at least at the time of bug #44007. But those issues have probably been fixed along the way in the 3-4 years since then.

Philip Nienhuis <philipnienhuis>
Group Member
Mon 21 Jan 2019 02:22:04 AM UTC, comment #15: 

Here's a patch that takes Sebastian's approach and adds BSD tar detection and support to both tar() and unpack(): file #46054:  tar-support-BSD-tar.patch

After this, it works on my Macs when either BSD or GNU tar is on the path as "tar". Tests for tar, untar, and unpack all pass.

This patch was made against stable.

Andrew Janke <apjanke>
Wed 27 Jun 2018 05:04:54 PM UTC, comment #14: 

This is more complicated than I expected. Without checking which tar (bsd/gnu) we cannot figure out if the output "a xyz" of tar means
1. the file with name "xyz" was added by bdstar to the archive or
2. the file with name "a xyz" was added by gnutar to the archive

So we have to check what tar we are actually using. The solution attached is not elegant but it works...

Sebastian


(file #44445)

Sebastian <sschoeps>
Sun 24 Jun 2018 09:09:13 PM UTC, comment #13: 

@sschoeps: I patched octave to use "gtar" as you suggested, and it worked.


diff --git a/scripts/miscellaneous/tar.m b/scripts/miscellaneous/tar.m
index 6d73eef..d516830 100644
--- a/scripts/miscellaneous/tar.m
+++ b/scripts/miscellaneous/tar.m
@@ -60,7 +60,7 @@ function filelist = tar (tarfile, files, rootdir = ".")
     tarfile = __w2mpth__ (tarfile);
   endif

-  cmd = sprintf ("tar cvf %s -C %s %s",
+  cmd = sprintf ("gtar cvf %s -C %s %s",
                           tarfile, rootdir, sprintf (" %s", files{:}));

   ## Save and restore the TAR_OPTIONS environment variable used by GNU tar.
diff --git a/scripts/miscellaneous/unpack.m b/scripts/miscellaneous/unpack.m
index 56ea975..3eba050 100644
--- a/scripts/miscellaneous/unpack.m
+++ b/scripts/miscellaneous/unpack.m
@@ -174,8 +174,8 @@ function filelist = unpack (file, dir = ".", filetype = "")
                        'bzip2 -d -f "%s"', ...
                        @__parse_bzip2__, true};
     commandlist.bz = commandlist.bz2;
-    commandlist.tar = {'tar xvf "%s"', ...
-                       'tar xf "%s"', ...
+    commandlist.tar = {'gtar xvf "%s"', ...
+                       'gtar xf "%s"', ...
                        @__parse_tar__, false};
     commandlist.targz = {'gzip -d -c "%s" | tar xvf -', ...
                          'gzip -d -c "%s" | tar xf -', ...


Test is passing now.


>> test tar
PASSES 6 out of 6 tests
>> test unpack
PASSES 9 out of 9 tests


Note that this patch is a hack specific to Octave.app on macOS; it is not a general fix for this bug.

Andrew Janke <apjanke>
Sun 24 Jun 2018 04:03:00 PM UTC, comment #12: 

I am not working on this at the moment.

I still believe that the best solution is #3, to make the tar.m and unpack.m functions work with any program called 'tar'.

Mike Miller <mtmiller>
Group Member
Sun 24 Jun 2018 03:36:32 PM UTC, comment #11: 

Is anyone working on it?

I think I can fix the regexp if I redirect stderr. This should work on linux/mac but windows would need to be treated separately because there is no redirection as far as I know.

@Mike: do you have an opinion on how to proceed? We have three options...
1. configure check for gnutar
2. runtime option (like makeinfo)
3. try to fix regexp (with the drawback mentioned above)

@Andrew: for now we can patch unpack.m to use "gtar" instead of "tar" and add gnu-tar as homebrew dependency.

Sebastian <sschoeps>
Sun 24 Jun 2018 06:19:53 AM UTC, comment #10: 

Would you consider as an alternative requiring a GNU tar, but doing a configure-time check to locate it, and allowing a user override? As far as I know, GNU tar is available on most systems, even if it has to be installed at a non-default location or prefix (like `/usr/local/bin/gtar`), so it's not an onerous requirement.

I'm working on a macOS installation of GNU Octave and running in to the same problem, since the system-supplied tar on macOS is a BSD tar. It's easy to install GNU Tar as a supplemental tar, but I'd need some way to point GNU Octave at it.

A run time check might be even better, as long as there was a way to supply a run time override that pointed Octave at the right GNU tar. That would make it easier to package Octave for redistribution, to systems which may or may not have GNU tar installed, and may have it installed at different locations. Something like the `makeinfo_program` function that allows run time specification of the texinfo location.

Andrew Janke <apjanke>
Wed 02 May 2018 09:47:10 PM UTC, comment #9: 

Incidentally I'm pulling the FreeBSD source code to try to get a non-GNU sed to compare with, see if any of our sed patterns can be made more portable, or if we need to add a sed feature test to configure. Separate bug report forthcoming.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 09:41:53 PM UTC, comment #8: 

I wanted to do the same initially but then I thought that redirecting stderr might break windows compatibility. However, I will think about it tomorrow. It's bed time in Germany :)

Sebastian <sschoeps>
Wed 02 May 2018 09:35:24 PM UTC, comment #7: 

No I'm thinking we force stderr to stdout, and we parse the output with the regexp function to get the file list.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 09:32:17 PM UTC, comment #6: 

Ok, I will play around with bsd-tar and probably propose a patch. Would you recommend using a runtime check in the m-file to test if we are using bsd or gnu?

Sebastian <sschoeps>
Wed 02 May 2018 09:15:06 PM UTC, comment #5: 

Thanks for the confirmation.

I think it's a little different, since we do require GNU sed in some places to build Octave (which I would also be happy to fix if possible, file a bug?), but we don't put any requirement on the user for a particular sed when using Octave.

This is different as it extends to the end user of the tar/untar/unpack functions. I think it's important to define the impact between the admin/build user and the end user.

Mike Miller <mtmiller>
Group Member
Wed 02 May 2018 08:44:56 PM UTC, comment #4: 

I think we do require also gnu-sed [1], so it would make sense to require gnu-tar but then we should list it as a dependency and then check for it during configure-time.

Sebastian

[1] https://lists.gnu.org/archive/html/octave-maintainers/2016-09/msg00193.html

Sebastian <sschoeps>
Thu 19 Apr 2018 04:33:46 PM UTC, comment #3: 

I have only found bug #43979 and bug #44007, both closed, both about similar test failures, but they were primarily because the programs in question didn't work well with forward-slash-separated file names.

Bug #44007 has some mention of GNU tar vs BSD tar, but it's in context of compiling one or the other within mxe-octave. And that seems to have been ditched in favor of continuing to use a pre-compiled GNU win32 tar. That might be what you are remembering.

I want to make sure the tar, untar, and unpack functions are more robust and compatible with either tar program.

Mike Miller <mtmiller>
Group Member
Thu 19 Apr 2018 04:57:41 AM UTC, comment #2: 

I think we came across this issue earlier.  All that means is there might be an existing bug report, since we didn't actually have a solution for it.

Rik <rik5>
Group administrator
Wed 18 Apr 2018 10:51:43 PM UTC, comment #1: 

The same applies for unpack.m with a tar file.

Mike Miller <mtmiller>
Group Member
Wed 18 Apr 2018 10:49:19 PM UTC, original submission:  

The tar.m function only works with the output of GNU tar, in particular when parsing the output of the program to return a list of files.

This can be tested and worked on easily enough on a GNU system by installing the 'bsdtar' package and changing the function to invoke 'bsdtar' temporarily.

Ideally the tar.m function (and other functions that call tar) can be made to work with all variants of the tar utility.

With bsdtar on Debian:


>> test tar.m
a dir2
a dir2/file2
a file1
***** test
 ## test tar together with untar
 orig_dir = pwd ();
 unwind_protect
   dirname = tarname = outdir = "";
   dirname = tempname ();
   assert (mkdir (dirname));
   chdir (dirname);
   dirname2 = "dir2";
   assert (mkdir (dirname2));
   fname1 = "file1";
   fname2 = fullfile (dirname2, "file2");
   fid = fopen (fname1, "wt");
   assert (fid >= 0);
   fdisp (fid, "Hello World");
   fclose (fid);
   fid = fopen (fname2, "wt");
   assert (fid >= 0);
   fdisp (fid, "Goodbye World");
   fclose (fid);
   tarname = [tempname() ".tar"];
   filelist = tar (tarname, {dirname2, fname1});
   if (! strcmp (filelist{3}, fname1))
     error ("tar file contents does not match expected file");
   endif
   if (! exist (tarname, "file"))
     error ("tar archive file cannot be found!");
   endif
   outdir = tempname;
   untar (tarname, outdir);
   fid = fopen (fullfile (outdir, fname1), "rt");
   assert (fid >= 0);
   str = fgetl (fid);
   fclose (fid);
   assert (str, "Hello World");
   fid = fopen (fullfile (outdir, fname2), "rt");
   assert (fid >= 0);
   str = fgetl (fid);
   fclose (fid);
   assert (str, "Goodbye World");
 unwind_protect_cleanup
   chdir (orig_dir);
   unlink (tarname);
   confirm_recursive_rmdir (false, "local");
   if (exist (dirname))
     rmdir (dirname, "s");
   endif
   if (exist (outdir))
     rmdir (outdir, "s");
   endif
 end_unwind_protect
!!!!! test failed
filelist(3): out of bound 0


Two key differences, the '-v' output goes to stderr, and each line starts with a prefix of 'a' before the file name.

Confirmed test failure with the buildbot workers running on macOS.

Mike Miller <mtmiller>
Group Member

 

(Note: upload size limit is set to 16384 kB, after insertion of the required escape characters.)

Attach Files:
   
   
Comment:
   

Attached Files
file #46067:  tar-support-BSD-tar-v3.patch added by apjanke (5KiB - application/octet-stream)
file #46061:  tar-support-BSD-tar-v2.patch added by apjanke (5KiB - application/octet-stream)
file #46054:  tar-support-BSD-tar.patch added by apjanke (4KiB - application/octet-stream)
file #44445:  tar.m added by sschoeps (5KiB - application/octet-stream)

 

Depends on the following items: None found

Items that depend on this one: None found

 

Carbon-Copy List
  • -email is unavailable- added by apjanke (Posted a comment)
  • -email is unavailable- added by sschoeps (Posted a comment)
  • -email is unavailable- added by rik5 (Posted a comment)
  •  

    There are 0 votes so far. Votes easily highlight which items people would like to see resolved in priority, independently of the priority of the item set by tracker managers.

    Only group members can vote.

     

    Follow 10 latest changes.

    Date Changed by Updated Field Previous Value => Replaced by
    2019-08-07 mtmiller StatusPatch Submitted Fixed
        Open/ClosedOpen Closed
    2019-08-06 mtmiller StatusPatch Reviewed Patch Submitted
    2019-01-22 apjanke Attached File- Added tar-support-BSD-tar-v3.patch, #46067
    2019-01-21 apjanke Attached File- Added tar-support-BSD-tar-v2.patch, #46061
    2019-01-21 mtmiller StatusConfirmed Patch Reviewed
    2019-01-21 mtmiller Carbon-CopyRemoved 80942 -
    2019-01-21 apjanke Attached File- Added tar-support-BSD-tar.patch, #46054
    2018-06-27 sschoeps Attached File- Added tar.m, #44445
    2018-04-18 mtmiller Summarytar.m is incompatible with BSD tar tar.m and unpack.m are incompatible with BSD tar

    Back to the top

    Powered by Savane 3.13-f8d8.
    Corresponding source code