WinRPM.jl
WinRPM.jl copied to clipboard
JuliaPackaging
check for unescaped cpio file
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. On point though: I am not clear on the reason the downloaded RPM file needs to be saved with an escaped name in the first place. In my opinion it is not needed on Windows, cannot speak for other OS's. When extracting the rpm file, I honestly cannot see why the files would have escaped names (the cpio file in my case), unless 7z would do this behind the scenes, again this might be OS dependent...
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. If for any other file/operating system that escape is necessary I can see an issue with the extraction of that rpm file, as the file that will come out of the compressed archive (the cpio file) would also need escaping and thus that extracting would fail. Either way, imho, the cpio file resulting from the rpm extraction will not be escaped. So I would be fine with removing the check for existence. Again this was only done to be safe
If for any other file/operating system that escape is necessary I can see an issue with the extraction of that rpm file, as the file that will come out of the compressed archive (the cpio file) would also need escaping and thus that extracting would fail.
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 / and \0. Windows is more restrictive than that.
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. Now we are specifying the file by name. I can see no reason why the cpio file would be escaped by extracting it out of the rpm archive. So I would just remove the file-exists check and progress by testing on a number of OS's. As a consequence though, I see no reason why the rpm file should be escaped, if the CPIO file can be exist, than the RPM file should as well, as the assumption is the filename are the same.
Are you sure the escaping was introduced by https://github.com/JuliaPackaging/WinRPM.jl/commit/70333a54cfba0db936cc67c1244bf54572b919a3? AFAICT it was already present in the original version of the code.
@vtjnash Since you wrote that code, do you have an opinion on this?
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 The options I see:
- use this PR, yes the checking for existence is not clean (euphemism), but it will most certainly work. 2.remove the file check, but use the unescaped file, as explained above I see now reason why you need to look for the escaped cpio file. But will take it upon them to test all file systems?
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. the C++ package is a good example. Anyone has some other packages that have names with characters to be escaped? I will include the test in the PR, will take me a little time as I have not created any Julia tests up to now, but its a valuable exercise. I can only test on Windows though
Thanks. Would it be possible to test something more specific than @test true at the end?
I can check the file is downloaded
On Thu, Oct 18, 2018 at 04:20 Milan Bouchet-Valat [email protected] 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 https://github.com/JuliaPackaging/WinRPM.jl/pull/157#issuecomment-430937883, or mute the thread https://github.com/notifications/unsubscribe-auth/Afj4KtOsPq0iSdbPpiGdfMlWIbyWSscNks5umEfzgaJpZM4WFzJA .
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 < [email protected]>:
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 https://github.com/JuliaPackaging/WinRPM.jl/pull/157#issuecomment-430987815, 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. 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 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 [email protected] 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 https://github.com/JuliaPackaging/WinRPM.jl/pull/157#issuecomment-431216890, or mute the thread https://github.com/notifications/unsubscribe-auth/Afj4KgJwOJEWTdL8Vms5iP1xmnix0lpAks5umSx7gaJpZM4WFzJA .
You don't have to apology, I also switched to v1.0 recently. :)
https://github.com/JuliaLang/Compat.jl/issues/603
I found that Compat.jl does not support iterate(iter, state) yet. So, I'm not sure, If there is no problem with v0.7, could we merge this pr first and later fix iterate in another pr?
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.
https://github.com/JuliaPackaging/WinRPM.jl/pull/158 We already have one! And it does not seem to conflict with this pr.
@nalimilan I added some extra test. As there were no tests in the past and it seems multiple people are looking for this solution, I hope we can push it through now.