Fix issue with .7 where 7zip extracts the file without the noarch%2F on it (Take2) #141

RandomString123 · 2018-02-16T18:08:46Z

This should fix #134 as well as any issue where WinRPM was failing to find the extracted cpio file from an rpm file. See #134 for a full discussion of why this was happening in .7.

This along with #136 & #139 should restore WinRPM to working condition in .7. However, the package produces a large number of warnings and a deprecation errors so this might change as .7 matures.

I am not sure if my @static if VERSION < v"0.7.0-DEV" is the proper way to fence off .7 code from .6 code. If not please let me know. I have tested this on both .7 and .6 and with this code in place files download, extract, and install successfully.

After looking this fix might be a duplicate of #137 as the problem looks the same but I did not look in depth at it.

tkelman · 2018-02-16T18:24:19Z

src/WinRPM.jl

@@ -463,7 +463,13 @@ function do_install(package::Package)
        write(f, data[1])
    end
    info("Extracting: ", name)
-    cpio = splitext(path2)[1]*".cpio"
+
+    @static if VERSION < v"0.7.0-DEV"


this should be specific to the merge commit that upgraded 7zip

I agree. but my newness to Julia means I need some help here. Is there an easy way to find that other than digging through hundreds of commits?

there's a commit-name script in contrib of a julia checkout, run it on JuliaLang/julia@8153470

tkelman · 2018-02-16T19:11:00Z

src/WinRPM.jl

@@ -464,7 +464,7 @@ function do_install(package::Package)
    end
    info("Extracting: ", name)

-    @static if VERSION < v"0.7.0-DEV"
+    @static if VERSION < v"0.7.0-DEV.2181"


the @static isn't really necessary

tkelman · 2018-02-16T19:20:12Z

Looks okay if you've tested it. But as with #137, wouldn't basename be a cleaner way of accomplishing the last(split(...)) ?

RandomString123 · 2018-02-16T19:35:27Z

Switched to basename(). I also verified code as of 98878bd still downloads and extracts files without errors on my .7 and .6 installs.

amellnik · 2018-02-16T20:29:20Z

This approach also handles filenames containing + / %2B, correct?

RandomString123 · 2018-02-16T20:45:54Z

I don't handle the filenames any differently than the code that was in place does. The only change that has been made is that any directory parts of the filename are now chopped off I.E. if the file is named noarch/happy_package.1.1.1.rpm I trim that to just be happy_package.1.1.1.rpm. So if a file was named noarch/package+%2B-1.1.1.rpm I would change it to package+%2B-1.1.1.rpm.

The old code was in essence (combining two lines into one):
cpio = splitext(joinpath(cache,escape(path)))[1]*".cpio"
My code is:
cpio = splitext(joinpath(cache, escape(basename(path))))[1] * ".cpio"

nalimilan · 2018-02-17T13:05:29Z

Thanks. If you've tested it and it works, looks good to me. @tkelman Fine with you too?

tbeason · 2018-02-20T19:06:41Z

This does look like a duplicate of #137 but is perhaps better than what I had done, I don't know. I ran this and it worked for me on v0.7 (combined with #136).

tkelman · 2018-02-20T19:56:22Z

Unlike #137, this looks like it should avoid breaking anything on 0.6

nalimilan · 2018-02-20T19:57:39Z

Meaning, OK to merge? :-)

musm · 2018-03-25T18:32:23Z

bump hdf5 build fails without this patch

musm · 2018-03-28T19:34:41Z

would also be good to tag after this

nalimilan · 2018-03-29T09:07:28Z

@tkelman Bump? I'm afraid you're the only one which knows this package enough. :-)

tkelman · 2018-03-30T23:26:51Z

Should really set up appveyor here to be sure but this is likely safe in its current form.

musm · 2018-03-30T23:32:56Z

Agreed. I did test locally that this pr worked. BTW i did try to get appveyor, but didn't add a proper cleaning step. I could look into that again, but no guarantees on when. For reference #95

nalimilan · 2018-03-31T10:04:31Z

OK, merging since anyway this doesn't change the behavior on 0.6, and it can't be worse on 0.7.

jcheyns · 2018-08-01T22:27:34Z

I still have the issue also mentioned by @amellnik
When installing Cbc I run into the following issue:
the rpm file is called

noarch%2Fmingw64-libstdc%2B%2B6-7.2.0-3.2.noarch.rpm
The filename got escaped, due to this statement
path2 = joinpath(cache,escape(path))

However the file in that archive is called

mingw64-libstdc++6-7.2.0-3.2.noarch.rpm
When determing the cpio filename, it is assume this is the same file name, which is not correct as it does not take the escaping into account, indeed for .7 we have
cpio = splitext(joinpath(cache, escape(basename(path))))[1] * ".cpio"
this should be replaced with
cpio = splitext(joinpath(cache, basename(path)))[1] * ".cpio"
In my local version this does the trick.

jcheyns · 2018-08-13T20:27:51Z

@nalimilan @tkelman Can you have a look at my comment above, this kinda stops my 0.7/1.0 transition ?

tkelman · 2018-08-13T20:37:24Z

Dunno. What changed?

jcheyns · 2018-08-13T20:55:59Z

in the download from the filename gets escaped. Then the compressed file is unpacked and it looks for .cpio. however in the compressed file the filename has not been escaped.
I'll give an example, during the building of Cbc http://download.opensuse.org/repositories/windows:/mingw:/win64/openSUSE_13.2/noarch/mingw64-libstdc++6-7.2.0-3.2.noarch.rpm
gets downloaded to a local file called
noarch%2Fmingw64-libstdc%2B%2B6-7.2.0-3.2.noarch
in the cache folder
the 7zip then is called to unpack file mingw64-libstdc%2B%2B6-7.2.0-3.2.noarch.cpio
but that file does not exists, it is called mingw64-libstdc++6-7.2.0-3.2.noarch.cpio

tkelman · 2018-08-13T20:59:59Z

13.2 is very old and shouldn't be referenced any more... the escaping must've been added for a reason and presumably worked and was necessary before?

jcheyns · 2018-08-13T22:22:41Z

My bad, I just grabbed a random SUSE version to make my case, CBC uses 42.2 actually.
I do not know exactly why the downloaded file needs the escaping, I know windows does not need it, not versed enough in other OS's to be say why.
The behavior of 7z seems to have changed in 0.7/1.0. In 0.6 using the wrong filename for extraction did still work, it does no longer in 0.7/1.0. I have read a reference to this somewhere, will dig it up and post when I found it.

tkelman · 2018-08-13T22:37:57Z

There was a change in 7z version and behavior between julia 0.6 and julia 0.7, but that happened before February/March when this PR was worked on. I'm under the assumption the author and other commenters on this PR had tested it on 0.7-DEV at that point. I'm asking what changed since February.

RandomString123 · 2018-08-13T23:10:50Z

I tested this with a .6 and .7 later than 2181, when the 7 zip version changed. The behavior described exactly matches what this PR fixes. Are you accidentally using a cached version of WinRPM prior to when this merge happened?

jcheyns · 2018-08-20T19:22:37Z

I have cleared everything, problem persists

As you can see , the cpio file is extracted correctly from the RPM, however the name is not escaped (note the ++)
And here is where things go wrong (line 467)

info("Extracting: ", name)
cpio = splitext(joinpath(cache, escape(basename(path))))[1] * ".cpio"

The file name cpio is built using an escaped name, the unzipped cpio file is unescaped, this then leads to the extract command $exe7z x -y $cpio -o$installdir failing

RandomString123 · 2018-08-20T19:58:06Z

I see what you are saying. The patch fixed all the other cases because the filenames did not have any special characters in them. However, your c++ filename gets mangled because it has html escape characters in it.

I agree the solution should be to just remove the escape() however I fear without testing this might break something else as this code is very arcane. My .7 Julia install is a bit out of date but I will see if I can get a new PR in with your proposed changes.

jcheyns · 2018-08-20T20:00:45Z

I am willing to do the PR.
One approach would be to test if the file exists (with and without escaping). It's not elegant, but for arcane code it might be an acceptable (=safe) approach.

RandomString123 · 2018-08-20T20:05:56Z

Sounds like a good approach that will ensure the file gets handled appropriately. I'll lave it to you to do the PR then.

jcheyns · 2018-08-20T20:18:49Z

OK, I'll tackle that. Not that much experience on git, so it's a good opportunity to learn something.

jcheyns · 2018-08-21T13:46:09Z

Made the PR #157

Search for just filename not name with %2F in it.

cb4317d

tkelman reviewed Feb 16, 2018

View reviewed changes

Target to specific version when 7zip changed 0.7.0-DEV.2181

fffb0bf

tkelman reviewed Feb 16, 2018

View reviewed changes

Remove static macro

c60d652

switch to basename

98878bd

tkelman mentioned this pull request Feb 28, 2018

07 support #143

Closed

musm mentioned this pull request Mar 27, 2018

update for 0.7 JuliaIO/HDF5.jl#468

Merged

nalimilan merged commit c2803c9 into JuliaPackaging:master Mar 31, 2018

jcheyns mentioned this pull request Aug 2, 2018

File not found error installing gcc-c++ on Julia v0.7.0-beta2 #150

Open

jcheyns mentioned this pull request Aug 21, 2018

check for unescaped cpio file #157

Open

larssp mentioned this pull request Sep 5, 2018

use unescape instead of escape for the cpio files #165

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix issue with .7 where 7zip extracts the file without the noarch%2F on it (Take2) #141

Fix issue with .7 where 7zip extracts the file without the noarch%2F on it (Take2) #141

RandomString123 commented Feb 16, 2018

tkelman Feb 16, 2018

RandomString123 Feb 16, 2018

tkelman Feb 16, 2018

tkelman Feb 16, 2018

tkelman commented Feb 16, 2018

RandomString123 commented Feb 16, 2018

amellnik commented Feb 16, 2018

RandomString123 commented Feb 16, 2018 •

edited

Loading

nalimilan commented Feb 17, 2018

tbeason commented Feb 20, 2018

tkelman commented Feb 20, 2018

nalimilan commented Feb 20, 2018

musm commented Mar 25, 2018

musm commented Mar 28, 2018

nalimilan commented Mar 29, 2018

tkelman commented Mar 30, 2018

musm commented Mar 30, 2018

nalimilan commented Mar 31, 2018

jcheyns commented Aug 1, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

RandomString123 commented Aug 13, 2018

jcheyns commented Aug 20, 2018

RandomString123 commented Aug 20, 2018

jcheyns commented Aug 20, 2018

RandomString123 commented Aug 20, 2018

jcheyns commented Aug 20, 2018

jcheyns commented Aug 21, 2018

Fix issue with .7 where 7zip extracts the file without the noarch%2F on it (Take2) #141

Fix issue with .7 where 7zip extracts the file without the noarch%2F on it (Take2) #141

Conversation

RandomString123 commented Feb 16, 2018

tkelman Feb 16, 2018

Choose a reason for hiding this comment

RandomString123 Feb 16, 2018

Choose a reason for hiding this comment

tkelman Feb 16, 2018

Choose a reason for hiding this comment

tkelman Feb 16, 2018

Choose a reason for hiding this comment

tkelman commented Feb 16, 2018

RandomString123 commented Feb 16, 2018

amellnik commented Feb 16, 2018

RandomString123 commented Feb 16, 2018 • edited Loading

nalimilan commented Feb 17, 2018

tbeason commented Feb 20, 2018

tkelman commented Feb 20, 2018

nalimilan commented Feb 20, 2018

musm commented Mar 25, 2018

musm commented Mar 28, 2018

nalimilan commented Mar 29, 2018

tkelman commented Mar 30, 2018

musm commented Mar 30, 2018

nalimilan commented Mar 31, 2018

jcheyns commented Aug 1, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

jcheyns commented Aug 13, 2018

tkelman commented Aug 13, 2018

RandomString123 commented Aug 13, 2018

jcheyns commented Aug 20, 2018

RandomString123 commented Aug 20, 2018

jcheyns commented Aug 20, 2018

RandomString123 commented Aug 20, 2018

jcheyns commented Aug 20, 2018

jcheyns commented Aug 21, 2018

RandomString123 commented Feb 16, 2018 •

edited

Loading