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

Added test integrity for the cfile compressors #225

Merged
merged 8 commits into from
Dec 28, 2018
Merged

Added test integrity for the cfile compressors #225

merged 8 commits into from
Dec 28, 2018

Conversation

stokito
Copy link
Contributor

@stokito stokito commented Oct 27, 2018

Added test integrity for the cfile compressors: gzip, bzip2, etc.
But since most of them shows the message with file status to STDERR instead of STDOUT whe should show both in Test result window.

To validate:
Open *.gz file (or *.bz2, *.lzip...), then File/Test integrity. You should see output of test command.
The only one archive which doesn't support test is rzip.

Also in #225 contains support of test brotli but this needs for #224 to be merged first.

… since most of them shows the message with file status to STDERR instead of STDOUT whe should show both in Test result window.
@stokito stokito mentioned this pull request Oct 27, 2018
@lukefromdc lukefromdc requested a review from a team October 28, 2018 18:34
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.

I really don't know how to test this. engrampa master currently supports integrity testing on some filetypes, but the option to integrity test a .xz file was greyed out in both this PR and master. Integrity testing a .gz file worked the same in either one

@lukefromdc lukefromdc requested a review from a team October 29, 2018 21:03
@stokito
Copy link
Contributor Author

stokito commented Oct 29, 2018

I didn't understood what do you mean "test a .xz file was greyed out ". I know only one place to trigger the integrity: main menu/Archive/Test integrity
If the menu item is grayed then maybe that's because xz is not installed in your system.
Then try to install it. You can test it on brotli for example too.

@lukefromdc
Copy link
Member

I have .xz support on my system and use it all the time. The engrampa menubar's archive>test integrity menu item is the one I had greyed out. Same thing happened with xz in #226 for some reason, even though I can both pack and unpack .xz archives with no problem in Engrampa on my system.

@stokito
Copy link
Contributor Author

stokito commented Nov 1, 2018

Hi @lukefromdc
The Test Integrity menu item was grayed because this wasn't supported until this PR. If you still see grayed item then probably you didn't sudo make install or try to remove engrampa and engrampa-common from Ubuntu. The only one unsupported integrity is rzip, but it will show the menu item and on click show empty result dialog anyway.
Meanwhile I see that the xz package name referred in sources is not correct and created #227 to fix that.
So, could you accepts the PR and then related #226?

src/fr-command-cfile.c Outdated Show resolved Hide resolved
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.

It worked here with .gz, but it did't work with .br, I see in terminal:

Test integrity in unsupported for application/x-brotli

@lukefromdc
Copy link
Member

I am not running Ubuntu but rather Debian Unstable without any of Debian's default MATE packages. I have all of MATE locally compiled

@lukefromdc
Copy link
Member

This may depend on #227 in order to work for .xz files.

@lukefromdc
Copy link
Member

I just got this build error after pulling the branch:

  CCLD     engrampa
/usr/bin/ld: fr-command-cfile.o: in function `fr_command_cfile_add':
/home/luke/Desktop/engrampa/src/fr-command-cfile.c:238: undefined reference to `r_process_set_working_dir'
collect2: error: ld returned 1 exit status
/usr/bin/ld: fr-command-cfile.o: in function `fr_command_cfile_add':
/home/luke/Desktop/engrampa/src/fr-command-cfile.c:238: undefined reference to `r_process_set_working_dir'
collect2: error: ld returned 1 exit status

@stokito
Copy link
Contributor Author

stokito commented Nov 2, 2018

fixed, pls merge

@lukefromdc lukefromdc requested review from lukefromdc and a team and removed request for lukefromdc November 2, 2018 22:43
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.

Builds and runs, though I still only get an integrity test menu option that is not greyed out for gzip (.zip) and brotl(no output results) and not for xz or tar.bzip2 files. Might be something local to my system, but be advised this is something I have not used other than for these tests and I am really NOT the best tester for thus due to the fact that I deal with only a few compressed file types in normal usage.

@lukefromdc
Copy link
Member

@raveit65 , @monsta , can someone else have a look at this?

@stokito
Copy link
Contributor Author

stokito commented Nov 2, 2018

tar.bzip2 and .zip are not affected by the change. And yes, brotli will show empty output until google/brotli#729 fixed.

src/fr-command-cfile.c Outdated Show resolved Hide resolved
@stokito
Copy link
Contributor Author

stokito commented Nov 10, 2018

This branch is ready to be merged. Is anything needs to be done? Thanks

@lukefromdc
Copy link
Member

No idea. @raveit65 or @monsta might know more

Copy link
Member

@vkareh vkareh left a comment

Choose a reason for hiding this comment

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

This builds and runs fine for me. I tested using gzip and xz and they both now show the Test Integrity option:
screenshot at 2018-12-22 07-05-48

@vkareh vkareh requested a review from a team December 22, 2018 12:12
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.

I just retested this, integrity test for .zip and .xz files now works, while it does not with master as of 12-22-2018. A tar.gz or tar.xz files does NOT offer an integrity test, but I am assuming that is a matter for another PR requiring different coding. That is because this PR only mentions the compressors themselves.

memory management in fr_window_view_last_output_print looks OK. char *utf8_line is freed, and char *line is initialized to scan->data which I am presuming is in use elsewhere and thus should not be freed. This was the only place I noticed a new dynamically allocated variable.

Unless this is also intended to offer integrity testing of tar.(something) files, it looks ready to go

@lukefromdc lukefromdc requested a review from a team December 22, 2018 20:18
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.

looks ok

@raveit65 raveit65 merged commit 9771782 into mate-desktop:master Dec 28, 2018
wdlkmpx pushed a commit to wdlkmpx/engrampa that referenced this pull request Aug 14, 2019
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