-
Notifications
You must be signed in to change notification settings - Fork 44
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
check for unescaped cpio file #157
base: master
Are you sure you want to change the base?
Conversation
Thanks, but checking whether the file exists sounds weird. We should be able to know the name of the file in advance. Can we understand what escaping rules should be applied? |
I agree it is not the greatest solution, feels like trial an error, I came to this proposal/implementation because it guarantees it will not break anything. And the arcane nature of the code together with the lack of testing, drove me to this decision. It's not great but it's working. |
I think we need escaped name to get downloaded .cpio files but not for extracted .rpm files. Is there any part that manually escape the path of .rpm file? |
I think we need to understand exactly what is going on and what's the correct general solution before merging a fix. |
Well the first question I have is why we need to save the downloaded rpm file to an escaped file name (L463). I do not see the need for that, again I only speak for a Windows solution here. |
Wait, rpm file is used on RedHat based Linux. When I use CentOS or Fedora, pacakage managers such as yum and dnf download and save rpm cache files without escaping. So maybe we don't need to differ Windows and other OS platforms. (Of course we must test on them before merging it) |
Linux allows for any characters in file names except |
The introduction of the escaping was done 5 years ago in a changeset "more robust extraction". At that time the cpio issue was not present, as 7z was called with the -t option, i.e. extract all *.cpio files. |
@nalimilan you are correct, the escape has always been there |
How can we progress on this issue? It is holding back the use of e.g. Cbc in 0.7 (and 1.0). I needed to install this for a colleague and can't use the released WinRPM Package
|
I'd be tempted to just stop escaping. I don't really like the approach from this PR because it wouldn't work if escaping is really needed; if we think that, better stop doing it. Can you ensure that it works with a variety of packages? |
@nalimilan I can try to build a small test with some packages. Do you want a new PR or do I modify this PR, with a removal of the test for existence. |
Thanks. Updating this PR should be fine. |
src/WinRPM.jl
Outdated
@@ -468,6 +468,12 @@ function do_install(package::Package) | |||
|
|||
cpio = splitext(joinpath(cache, escape(basename(path))))[1] * ".cpio" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The filenames of the downloaded rpm packages are escaped (e.g. noarch%2Fmingw64-libstdc%2B%2B6-8.2.0-2.1.noarch.rpm
), but the cpio files after extraction with 7z are unescaped (e.g. mingw64-libstdc++6-8.2.0-2.1.noarch.cpio
).
I guess escape
in l.469 should be changed to unescape
since
julia> using URIParser
julia> escape("c++")
"c%2B%2B"
julia> unescape("c%2B%2B")
"c++"
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That makes sense to me, I wonder how it escaped us!
@jcheyns What do you think about it? Does it fix the bugs you encountered?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sure, that was the fix I had. But in the original discussion (#141) we came to the conclusion to but a safety net in there and first check for the existence of the file, as no one seems very confident about this archane package.
I have this mod ready and works locally for me, as discussed I also want to have a couple of tests.
need to find some time for that. I can check in the PR and add the test later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe I'm missing something, but #141 wasn't about using unescape
AFAICT, right?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes and no. I originally started this discussion there
Hope this PR now fixes things. |
Thanks. Would it be possible to test something more specific than |
I can check the file is downloaded
…On Thu, Oct 18, 2018 at 04:20 Milan Bouchet-Valat ***@***.***> wrote:
Thanks. Would it be possible to test something more specific than @test
true at the end?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#157 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Afj4KtOsPq0iSdbPpiGdfMlWIbyWSscNks5umEfzgaJpZM4WFzJA>
.
|
And can you check that it's been installed correctly? |
I can try, but not a specialist in the whole WRPM installation. I will
start with the check of the downloaded file.
that should be enough to get this PR pushed into main
Op do 18 okt. 2018 om 07:22 schreef Milan Bouchet-Valat <
notifications@github.com>:
… And can you check that it's been installed correctly?
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#157 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Afj4KqJmxBypkXhqqvd4M3A_ejpKSqfaks5umHKbgaJpZM4WFzJA>
.
|
I got this error: (v1.0) pkg> build WinRPM
Building LibCURL → `C:\Users\alkorang\.julia\packages\LibCURL\OoXMv\deps\build.log`
Building WinRPM ─→ `C:\Users\alkorang\.julia\dev\WinRPM\deps\build.log`
Resolving package versions...
┌ Error: Error building `WinRPM`:
│ ERROR: LoadError: UndefVarError: start not defined
│ Stacktrace:
│ [1] getproperty(::Module, ::Symbol) at .\sysimg.jl:13
│ [2] top-level scope at none:0
│ [3] include at .\boot.jl:317 [inlined]
│ [4] include_relative(::Module, ::String) at .\loading.jl:1041
│ [5] include(::Module, ::String) at .\sysimg.jl:29
│ [6] top-level scope at none:2
│ [7] eval at .\boot.jl:319 [inlined]
│ [8] eval(::Expr) at .\client.jl:389
│ [9] top-level scope at .\none:3
│ in expression starting at C:\Users\alkorang\.julia\dev\WinRPM\src\WinRPM.jl:214
│ ERROR: LoadError: Failed to precompile WinRPM [c17dfb99-b4f7-5aad-8812-456da1ad7187] to C:\Users\alkorang\.julia\compiled\v1.0\WinRPM\ko3j0.ji.
│ Stacktrace:
│ [1] error(::String) at .\error.jl:33
│ [2] macro expansion at .\logging.jl:313 [inlined]
│ [3] compilecache(::Base.PkgId, ::String) at .\loading.jl:1187
│ [4] _require(::Base.PkgId) at .\logging.jl:311
│ [5] require(::Base.PkgId) at .\loading.jl:855
│ [6] macro expansion at .\logging.jl:311 [inlined]
│ [7] require(::Module, ::Symbol) at .\loading.jl:837
│ [8] top-level scope at C:\Users\alkorang\.julia\dev\WinRPM\deps\build.jl:2
│ [9] include at .\boot.jl:317 [inlined]
│ [10] include_relative(::Module, ::String) at .\loading.jl:1041
│ [11] include(::Module, ::String) at .\sysimg.jl:29
│ [12] include(::String) at .\client.jl:388
│ [13] top-level scope at none:0
│ in expression starting at C:\Users\alkorang\.julia\dev\WinRPM\deps\build.jl:1
└ @ Pkg.Operations C:\cygwin\home\Administrator\buildbot\worker\package_win64\build\usr\share\julia\stdlib\v1.0\Pkg\src\Operations.jl:1069
(v1.0) pkg> |
Iteration interfaces have been changed in Julia v1.0. Fixing this may require later version of Compat.jl package. |
Apologies, I didn’t check this. I still live in 0.7
…On Thu, Oct 18, 2018 at 20:35 Yeonsoo Kim ***@***.***> wrote:
Iteration interfaces have been changed in Julia v1.0.
Julia v0.6:
https://docs.julialang.org/en/v0.6/manual/interfaces/#man-interface-iteration-1
Julia v1.0:
https://docs.julialang.org/en/v1.0/manual/interfaces/#man-interface-iteration-1
Fixing this may requires later version of Compat.jl package.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#157 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/Afj4KgJwOJEWTdL8Vms5iP1xmnix0lpAks5umSx7gaJpZM4WFzJA>
.
|
You don't have to apology, I also switched to v1.0 recently. :) JuliaLang/Compat.jl#603 |
The simplest solution is to drop 0.6 support and use the new interface (which also works on 0.7). But indeed that's for another PR. |
#158 |
@nalimilan I added some extra test. |
#check that a file with a special char is downloaded correctly | ||
@testset "escaping" begin | ||
pkgs = [WinRPM.select(WinRPM.lookup("libstdc++6", WinRPM.OS_ARCH), "libstdc++6"))] | ||
WinRPM.do_install(WinRPM.Packages(pkgs)) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
see #95 (comment) and https://github.com/JuliaPackaging/WinRPM.jl/pull/147/files/929e616ce9eac79c710d80640dba6946362064ca#diff-fce720c43af3c52c862fd7451c7374b8 - running tests should avoid making permanent observable modifications to a user's environment, such as installing a bunch of packages they may not need or want for anything other than verifying tests work. Better would be to do this in a temporary location that gets cleaned up afterwards.
fix #141