Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Mar 31, 2018

Conversation

RandomString123
Copy link
Contributor

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.

src/WinRPM.jl Outdated
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

src/WinRPM.jl Outdated
@@ -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"
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

the @static isn't really necessary

@tkelman
Copy link
Contributor

tkelman commented Feb 16, 2018

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

@RandomString123
Copy link
Contributor Author

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

@amellnik
Copy link

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

@RandomString123
Copy link
Contributor Author

RandomString123 commented Feb 16, 2018

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
Copy link
Member

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

@tbeason
Copy link
Contributor

tbeason commented Feb 20, 2018

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
Copy link
Contributor

tkelman commented Feb 20, 2018

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

@nalimilan
Copy link
Member

Meaning, OK to merge? :-)

@tkelman tkelman mentioned this pull request Feb 28, 2018
@musm
Copy link
Contributor

musm commented Mar 25, 2018

bump hdf5 build fails without this patch

@musm
Copy link
Contributor

musm commented Mar 28, 2018

would also be good to tag after this

@nalimilan
Copy link
Member

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

@tkelman
Copy link
Contributor

tkelman commented Mar 30, 2018

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

@musm
Copy link
Contributor

musm commented Mar 30, 2018

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 nalimilan merged commit c2803c9 into JuliaPackaging:master Mar 31, 2018
@nalimilan
Copy link
Member

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

@jcheyns
Copy link

jcheyns commented Aug 1, 2018

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
Copy link

jcheyns commented Aug 13, 2018

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

@tkelman
Copy link
Contributor

tkelman commented Aug 13, 2018

Dunno. What changed?

@jcheyns
Copy link

jcheyns commented Aug 13, 2018

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
Copy link
Contributor

tkelman commented Aug 13, 2018

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
Copy link

jcheyns commented Aug 13, 2018

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
Copy link
Contributor

tkelman commented Aug 13, 2018

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
Copy link
Contributor Author

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
Copy link

jcheyns commented Aug 20, 2018

I have cleared everything, problem persists
image
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
Copy link
Contributor Author

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
Copy link

jcheyns commented Aug 20, 2018

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
Copy link
Contributor Author

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
Copy link

jcheyns commented Aug 20, 2018

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

@jcheyns
Copy link

jcheyns commented Aug 21, 2018

Made the PR #157

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

build errors for JLD, HDF5, Cairo, SQLite
7 participants