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

Fixed xz package name #227

Closed
wants to merge 1 commit into from
Closed

Fixed xz package name #227

wants to merge 1 commit into from

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Nov 1, 2018

Copy link
Member

@lukefromdc lukefromdc left a comment

Choose a reason for hiding this comment

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

By itself this had no effect on Engrampa's behavior, with the XZ files still not being integrity testable . Might be one of the other PR's but all these interlocking PR's are too time consuming for me to separately test, then merge together for more testing at the moment

@stokito
Copy link
Contributor Author

stokito commented Nov 2, 2018

It really doesn't affect anything and I tested and xz files still working as previously. Safe to merge

@lukefromdc lukefromdc requested a review from a team November 2, 2018 22:09
@lukefromdc lukefromdc requested a review from a team November 2, 2018 22:15
Copy link
Member

@sc0w sc0w left a comment

Choose a reason for hiding this comment

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

if all is working as before, why we need that change?

@sc0w sc0w requested a review from a team November 4, 2018 02:13
Copy link
Member

@raveit65 raveit65 left a comment

Choose a reason for hiding this comment

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

I would not change that.

@stokito
Copy link
Contributor Author

stokito commented Nov 4, 2018

The packages.match file is used when you trying to open an archive which compressor is not installed in system so engrampa shows you a dialog window dlg-package-installer.c and asks you to install the compressor. But in fact xz-utils is present in almost all distros by default. For example it's used in .deb files. This may be changed in feature because, for example Ubuntu has plans to replace xz with zstd for packages.

@vkareh
Copy link
Member

vkareh commented Nov 8, 2018

@stokito - if xz-utils is not installed in your system, what does engrampa do when trying to open an xz package? Currently if I remove xz-utils, it tries to pull almost the entire system with it, so I cannot really test this properly...

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

Yes, it's not possible to remove xz-utils atm and the only distro without xz is OpenWrt but to see the behavior you can try to open, for example, any *.rar file and engrampa will ask you to install rar package.

So there is no issue today but it may become a problem in feature. I guess that you can even remove the line without new problems. But this is just a semantically error.

@vkareh
Copy link
Member

vkareh commented Nov 8, 2018

Okay, I created a throw-away VM and compiled engrampa with packagekit support. I removed xz-utils and tried to open an .xz file. On master I get an error that it cannot find xz package. On #227 I get a proper search for xz-utils (couldn't actually install it since removing xz-utils really messed up something in the VM, but it now searches for the correct package). This change seems correct, based on that.

@stokito - I noticed that in src/fr-command-tar.c, line 1121, you also need to update that one to xz-utils to account for application/x-xz-compressed-tar files. If you can make that change, I think this PR should be good to go.

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

@vkareh thanks, I pushed the fixed version

@raveit65
Copy link
Member

raveit65 commented Nov 8, 2018

i agree with @sc0w not to change that without any reason.

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

@raveit65 this is an error, so why not fix it? There is at least two reasons:

  1. it's semantically incorrect so can misleading someone who tries to understand the code: should I put here a program name or a package name? This is a real issue which I faced when tried to figure out this when added brotli support.
  2. In feature the xs-util package may be not present by default in system. Or Engrampa can be ported to other systems like MacOS or Windows where xz-utils isn't present.

@sc0w
Copy link
Member

sc0w commented Nov 8, 2018

Why the name of debian package?

In fedora its name is xz (https://apps.fedoraproject.org/packages/xz)

@vkareh
Copy link
Member

vkareh commented Nov 8, 2018

@sc0w - that's a good point, I didn't realize that. I wonder if it can be solved by changing to

return PACKAGES ("xz,xz-utils");

That way it would search for both. This seems like a slippery slope, though, as we'll never know all possible naming conventions for xz...

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

Yeah, looks like this differs between distros, I checked only in Ubuntu, Debian and OpenWrt and there it called xz-utils.
In Arch Linux it also called xz

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

Ok, so the last change I guess should satisfy everyone

@vkareh vkareh self-requested a review November 8, 2018 15:31
@raveit65
Copy link
Member

raveit65 commented Nov 8, 2018

Did anyone check the other 100 linux distros ? :)

@stokito
Copy link
Contributor Author

stokito commented Nov 8, 2018

Ok, then if it turned out that xz actually was a correct package name at least for Fedora and Arch then let's just close the PR.

@stokito stokito closed this Nov 8, 2018
@vkareh
Copy link
Member

vkareh commented Nov 8, 2018

Yes, okay, I've been looking at this all morning. Here's what I've found: the packages.match file seems to be meant for distros to override which package to look for. So, for Debian-based distros, their install scripts should end up having:

...
xunzip=
xz=xz-utils
zip=
...

The issue is that the current implementation of name lookup in engrampa is broken. It needs this commit for the name lookup to work:

@vkareh
Copy link
Member

vkareh commented Nov 8, 2018

The other thing is that most front-ends for PackageKit just do a search, so xz should also return xz-utils (along with possibly other results), so there shouldn't be an issue with leaving it as xz (it's been like this in file-roller for 8 years and distros haven't complained)

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.

5 participants