Extended workaround for handling symlinks in tar files #163

hhaensel · 2019-05-28T08:48:32Z

I propose another workaround additional to copyderef.

copyderef2 is automatically detected in case that neither tempdir nor dest supports symlinks.
In that case the symlinks of the tarfile are returned by the new function list_tarball_symlinks.
For this purpose the generic list functions have been modified to support a keyword argument verbose.
Regular expressions are used to parse the output and return a dictionary of symlinks.
The symlinks are exluded during extraction and the original source files of the symlinks are copied after extraction.

…out symlink support

hhaensel · 2019-05-28T09:00:51Z

I would be happy about people testing this on non-windows OS.
copyderef2 can be forced by an environment variable BINARYPROVIDER_COPYDEREF2, which needs to be set to true.
(ImageMagick uses symlinks ;-) )
Update:
Just realised that this functionality is tested by runtests.jl. But in order to work, line 777 needs to be modified:
"-" withenv("BINARYPROVIDER_COPYDEREF" => "true") do
"+" withenv("BINARYPROVIDER_COPYDEREF2" => "true") do

hhaensel · 2019-05-28T14:46:11Z

It seems as if the verbose output of the tar listing on Mac-OS is different to that of a linux shell.
Therefore, my regex fails to identify the symlinks :-(
Tomorrow I will know, how a listing looks like and possibly adapt my regex...

hhaensel · 2019-05-28T15:57:33Z

It seems I didn't have a proper example...

hhaensel · 2019-05-28T21:33:58Z

Looks ok for me now :-)

hhaensel · 2019-05-30T08:00:59Z

@staticfloat who is deciding on merging?
The regex should match most tar cases.

If a symlink starts with four numbers followed by a blank, it might be a problem ...
One idea to make it more robust is to create one tar list example by
echo a > a.txt && tar -cz a.txt | tar -tzv and then determine the date pattern of the tar output...

Even more robust would be to first unzip, add a marker file to the tar and then list the content...
But we could leave this up to later, if this comes up as a problem.

…gen_7z` in order to avoid the "world-age problem"

hhaensel · 2019-06-05T15:35:49Z

The last two changes were made to make the symlink parsing more robust as there are many formats around which might be even language dependent.

Idea
Check the listing of a demo file via `tar -c demoTar.txt | tar -tv`` and determine the number of parameters before the filename and finally construct a regex that filters the listing.

Note
gen_7z needs to contain the regex literally, because splicing into functions has a lot of pitfalls (world-age problem)
For the tar section this is not a problem, because the regex is spliced into the tuple as an element and not as part of a function.

I consider this the last version for this PR and would recommend merging into master.

staticfloat

If this works well (we have an explicit test, so if it passes tests, it should) we should just dump the first copyderef code path and use only this one; it is clearly superior in pretty much every way, so I suggest just deleting the old codepaths, getting rid of the 2 at the end of your settings, and calling it good!

Thank you for your effort, and I'm sorry that it took me so long to review this!

staticfloat · 2019-06-05T16:37:03Z

src/Prefix.jl

+        error("Could not list contents of tarball $(tarball_path)")
+    end
+    output = collect_stdout(oc)
+    # @info("copyderef2: \r\n" * output)


Zap this comment

staticfloat · 2019-06-05T16:39:12Z

src/PlatformEngines.jl

+    gen_7z = (p) -> (unpack_7z(p), package_7z(p), list_7z(p), parse_7z_list, r"Path = ([^\r\n]+)\r?\n(?:[^\r\n]+\r?\n)+Symbolic Link = ([^\r\n]+)"s)
    compression_engines = Tuple[]

+    write("demoTar.txt","Demo file for tar listing")


This should be a temporary file created with tempname(); otherwise we are potentially overwriting user files.

src/PlatformEngines.jl

…cument regex, modify excludeoption to also accept files with spaces

src/PlatformEngines.jl

…s accidentilly removed) and moved it to the correct location above `probe_symlink_creation(dest)` (was wrong before!)

staticfloat · 2019-06-06T16:37:49Z

src/PlatformEngines.jl

                Jjz = "j"
            end
-            return `$tar_cmd -x$(Jjz)f $(tarball_path) --directory=$(out_path) $(excludelist=="" ? [] : ["--exclude-from=" * excludelist])`
+            excludeoption = excludelist=="" ? `` : `--exclude-from='$excludelist'`


Don't use backticks when interpolating into other backticks; just use Strings.

src/PlatformEngines.jl

staticfloat · 2019-06-06T16:43:27Z

src/PlatformEngines.jl

    # the correct 7z given the path to the executable:
    unpack_7z = (exe7z) -> begin
-        return (tarball_path, out_path) ->
+        return (tarball_path, out_path, excludelist = "") ->


Let's default excludelist to nothing

staticfloat · 2019-06-06T16:43:46Z

src/PlatformEngines.jl

+        end
        # Some tar's aren't smart enough to auto-guess decompression method. :(
-        unpack_tar = (tarball_path, out_path) -> begin
+        unpack_tar = (tarball_path, out_path, excludelist = "") -> begin


Let's default excludelist to nothing

staticfloat · 2019-06-06T16:44:04Z

src/PlatformEngines.jl

+    # This is to work around filesystems that are mounted (such as SMBFS filesystems)
+    # that do not support symlinks.
+
+    excludelist = ""


default excludelist to nothing

I digged a bit into interpolation from commands and I think I now understand the behavior:

x = `` `command -o$x` # will evaluate to `command` and drop the option completely x = "my file" `command -o$x` # will evaluate to `command '-omy file'`

So what about defaulting excludelist::Union{AbstractString, Cmd} = ``
Then we can skip the excludelist == nothing ? ... : ... and simply use

pipeline(`$exe7z x $(tarball_path) -y -so`, `$exe7z x -si -y -ttar -o$(out_path) -x@$(excludelist)`)

and

return `$tar_cmd -x$(Jjz)f $(tarball_path) --directory=$(out_path) --exclude-from=$(excludelist)`

I rewrote it and it works like a charm ...

So what's happening here is a kind of surprising interaction in the Cmd parser where it's seeing command -o$x as a cartesian product between arrays to be expanded, as described in the docs here. Essentially, it looks at the Cmd like a container, and tries to iterate over it, but the empty Cmd is seen as a zero-dimensional object, and this disables all iteration, so we never see the first part (the "-o").

This is not very intuitive at all, and I would not be surprised if this changes in a future version of Julia, so I think it's best not to rely on it. Let's not use Cmd objects at all when interpolating into other Cmd objects and only use Strings; that's what is documented best, and is least likely to change in a future version.

Using nothing as the sentinel value for "do nothing" is pretty idiomatic, so I think thats better than using something else like a Cmd or an empty string; it fits in better with the rest of the APIs in Julia.

Thanks for putting so much work into this!

src/PlatformEngines.jl

…} and defaulting it to ``

…ws 10: option "-f -" for stdin/stdout, removed \r in parse_tar_list

hhaensel · 2019-06-08T01:24:18Z

I can easily agree to your comment, so I reverted my changes.

During testing I realised that Windows 10 has its own tar command which outputs \r\n.
Therefore, I have removed trailing \r in parse_tar_list, I hope that's ok.
Alternatively, the parse_tar_list could be done with a regex.
lines = [m.captures[1] for m in eachmatch(r"^(.+?)(?<!/\r|/)\r?$"m, output)]
For 7z this is also possible with
r".+:\d{2}\s+\.\D+\d+\D+\d+\s+(?:\./|\.\)?(.+?)\r?$"m

The tests at 603-605 of runtests.jl don't pass if one choses tar or busybox tar in Windows, since

@test joinpath("bin", "bar.sh") in contents

fails due to different fileseparators. One could do something like

joinpath("etc/qux.conf") in replace.(contents, ["\\" => "/"])

but I see that it's less elegant ;-)

So please comment, whether these two changes (regex and runtest.jl) should be made.
Otherwise, I hope that we are done with this PR...

staticfloat · 2019-06-10T21:34:37Z

src/PlatformEngines.jl


 """
-    gen_unpack_cmd(tarball_path::AbstractString, out_path::AbstractString; excludelist::Union{AbstractString, Cmd} = ``)
+    gen_unpack_cmd(tarball_path::AbstractString, out_path::AbstractString; excludelist::AbstractString = nothing)


nothing is not an AbstractString, so this won't work; you need to have excludelist be a Union{AbstractString, Nothing}.

staticfloat · 2019-06-10T21:34:51Z

src/PlatformEngines.jl

 automatically called upon first import of `BinaryProvider`.
 """
-gen_unpack_cmd = (tarball_path::AbstractString, out_path::AbstractString; excludelist::Union{AbstractString, Cmd} = ``) ->
+gen_unpack_cmd = (tarball_path::AbstractString, out_path::AbstractString; excludelist::AbstractString = nothing) ->


Stupid me! Thanks for your vigilance and patience - it's my first PR ;-)

You're doing a great job! Keep up the good work!

staticfloat · 2019-06-10T21:58:49Z

I think the cleanest solution for the file path separation issue on Windows is to translate the filenames to be consistent across all platforms within list_tarball_files(). So far, we've been using UNIX-y tools so this hasn't been a problem, but now that we have Windows-native tools, we need to check. :P So let's just have list_tarball_files always use / since that is valid on all platforms. You can split the filenames by "\\" and then join them with "/" and return that.

hhaensel · 2019-06-10T22:01:26Z

Oops, needs some rework, probably tomorrow ...

Just to make my point clear: The tar-listing is ok and it is formatted with "/". The test
@test joinpath("bin", "bar.sh") in contents fails with the windows tar, because the tar uses / and joinpath uses \\

hhaensel · 2019-06-10T22:50:56Z

The regex that I had unintentionally left in a modified version of tar_list should have been
r"^(?:\./|\.\\)?(.+?)(?<!/|/\r)\r?$"m ...
That would have worked. But the existing version is longer code but probably faster, so I left it unchanged except the removal of the \r line endings.

hhaensel · 2019-06-11T08:26:19Z

I just realised that there is no mechanism for erroring out in case that probe_platform_engines!() has not been run.
What do you think about adding

    if !isdefined(BinaryProvider, :gen_symlink_parser)
        error("Call `probe_platform_engines!()` before `list_tarball_symlinks()`")
    end

Btw, all error messages in the existing code still use probe_platform_engines() instead of probe_platform_engines!()
Shall I replace these within that PR?

Also, if you find my regex explanations too verbose, I might cut them down. Or feel free to redact them yourself to your own taste.

staticfloat

Yes, I think we should make the error messages and error conditions consistent, but let's do that in a separate PR. Put the correct error message within this new method you've added here, but then correct the other things in a separate PR.

staticfloat · 2019-06-11T21:13:58Z

src/PlatformEngines.jl


-# Global variable that tells us whether tempdir() can have symlinks
-# created within it.
-tempdir_symlink_creation = false


Every time a global is removed from a file, an angel sheds a single tear of joy.

staticfloat · 2019-06-11T21:23:01Z

I'm not certain why the tests are failing on Windows. I may need to dig into that; it's possible this symlink thing isn't working for one of the test cases, but I don't have time to dig into it right now. If you have access to a windows machine and can run the test cases locally, that might help.

hhaensel · 2019-06-12T05:41:05Z

I'm not certain why the tests are failing on Windows. I may need to dig into that; it's possible this symlink thing isn't working for one of the test cases, but I don't have time to dig into it right now. If you have access to a windows machine and can run the test cases locally, that might help.

Tests are failing only, if I manually chose tar to be the compression engine. Tar on windows behaves like a regular unix-y tar and lists its contents with regular slashes, whereas 7z uses backslashes.
We could change that behavior but I can't judge, whether this would do something unexpected at a different place. In my symlinks workaround I need / for listing in excludelist but I use \ for the copying ...
Anyhow, I will now push the final version of this PR.

staticfloat · 2019-06-12T15:40:23Z

Thanks @hhaensel! I will correct the / vs. \ problems in another PR. This has been a long time coming, but I'm glad you stuck it through!

hhaensel · 2019-06-12T16:23:19Z

I am really happy to see this merged - and I am a bit proud, as this is my first PR ever :-)
@staticfloat Thanks for your support! I learned a lot from the discussion we had.

Helmut Haensel added 3 commits May 28, 2019 10:08

Extended workaround for tars containing symlinks in environments with…

e583739

…out symlink support

Remove earlier windows workaround

affeb14

slight formatting correction

388f7f9

hhaensel mentioned this pull request May 28, 2019

Symlink warning on Windows 10 #149

Closed

corrected tar command for verbose listing

6870e24

hhaensel mentioned this pull request May 28, 2019

Cannot be build on Julia v1.0 JuliaIO/ImageMagick.jl#133

Closed

another correction of the tar command

2f7a92c

Adapted regex to match tar listing of mac-os

a03bf43

hhaensel added 3 commits May 28, 2019 21:40

obtain debug info for tuning the regex

443fda4

More debug info

14f2f9f

Adapt regex for Mac OS, next version

d715d0f

New version of regex to match more tars

2cdf50e

Helmut Haensel added 3 commits June 5, 2019 15:29

Determine tarlist format from a demo file listing. Slight change to `…

3715ed9

…gen_7z` in order to avoid the "world-age problem"

slight modification to avoid changes to gen_7z

ed75abc

define symlink_parser locally

e3cc652

staticfloat suggested changes Jun 5, 2019

View reviewed changes

Replace old copyderef by new version, use mktemp() for temp files, do…

d25781e

…cument regex, modify excludeoption to also accept files with spaces

staticfloat suggested changes Jun 6, 2019

View reviewed changes

src/PlatformEngines.jl Show resolved Hide resolved

src/PlatformEngines.jl Show resolved Hide resolved

Helmut Haensel added 2 commits June 6, 2019 09:48

added examples of verbose listings, put back mkpath(dest) (which wa…

7647815

…s accidentilly removed) and moved it to the correct location above `probe_symlink_creation(dest)` (was wrong before!)

Added 7z Listing and corrected tar exclude command

24208e8

staticfloat suggested changes Jun 6, 2019

View reviewed changes

Helmut Haensel and others added 2 commits June 7, 2019 15:24

brushing up the code and making excludlist::Union{AbstractString, Cmd…

02019ae

…} and defaulting it to ``

excludelist defaults to nothing; adaptations for tar command in Windo…

469ec23

…ws 10: option "-f -" for stdin/stdout, removed \r in parse_tar_list

staticfloat suggested changes Jun 10, 2019

View reviewed changes

Change type of excludelist argument to Union{AbstractString, Nothing}

8fe998a

hhaensel added 3 commits June 11, 2019 00:23

Change type of excludelist argument to Union{AbstractString, Nothing}

6f74c12

Merge branch 'master' of https://github.com/hhaensel/BinaryProvider.jl

8d77e76

Correct some errors...

74e2cdb

staticfloat reviewed Jun 11, 2019

View reviewed changes

place an error in case that probe_platform_engines!() has not been run

e30fd37

staticfloat merged commit 6b34650 into JuliaPackaging:master Jun 12, 2019

hhaensel mentioned this pull request Jun 17, 2019

Symlinks on windows, again. JuliaIO/LibpngBuilder#8

Open

Extended workaround for handling symlinks in tar files #163

Extended workaround for handling symlinks in tar files #163

Uh oh!

Conversation

hhaensel commented May 28, 2019

Uh oh!

hhaensel commented May 28, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hhaensel commented May 28, 2019

Uh oh!

hhaensel commented May 28, 2019

Uh oh!

hhaensel commented May 28, 2019

Uh oh!

hhaensel commented May 30, 2019

Uh oh!

hhaensel commented Jun 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staticfloat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

hhaensel Jun 7, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

hhaensel commented Jun 8, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Jun 10, 2019

Uh oh!

hhaensel commented Jun 10, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

hhaensel commented Jun 10, 2019

Uh oh!

hhaensel commented Jun 11, 2019

Uh oh!

staticfloat left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

staticfloat commented Jun 11, 2019

Uh oh!

hhaensel commented Jun 12, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

staticfloat commented Jun 12, 2019

Uh oh!

hhaensel commented May 28, 2019 •

edited

Loading

hhaensel commented Jun 5, 2019 •

edited

Loading

hhaensel Jun 7, 2019 •

edited

Loading

hhaensel commented Jun 8, 2019 •

edited

Loading

hhaensel commented Jun 10, 2019 •

edited

Loading

hhaensel commented Jun 12, 2019 •

edited

Loading