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 exiv2: verbose extract stdout mutli-file #2068

Merged
merged 4 commits into from
Feb 5, 2022

Conversation

postscript-dev
Copy link
Collaborator

When using exiv2 --verbose, --extract with stdout and multiple files, the output is concatenated together.

When using `exiv2 --verbose`, `--extract` with stdout and multiple
files, the output is concatenated together.
@postscript-dev postscript-dev added this to the v1.00 milestone Feb 4, 2022
@postscript-dev postscript-dev self-assigned this Feb 4, 2022
@codecov
Copy link

codecov bot commented Feb 4, 2022

Codecov Report

Merging #2068 (8ac0be3) into main (8505f4d) will decrease coverage by 55.76%.
The diff coverage is 0.00%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #2068       +/-   ##
==========================================
- Coverage   61.48%   5.72%   -55.77%     
==========================================
  Files          96      96               
  Lines       19207   18958      -249     
  Branches     9843    9845        +2     
==========================================
- Hits        11810    1085    -10725     
- Misses       5080   17685    +12605     
+ Partials     2317     188     -2129     
Impacted Files Coverage Δ
src/exiv2.cpp 0.00% <0.00%> (-59.52%) ⬇️
src/getopt.hpp 0.00% <0.00%> (-100.00%) ⬇️
src/actions.hpp 0.00% <0.00%> (-100.00%) ⬇️
src/exiv2app.hpp 0.00% <0.00%> (-100.00%) ⬇️
src/fujimn_int.cpp 0.00% <0.00%> (-100.00%) ⬇️
src/orfimage_int.hpp 0.00% <0.00%> (-100.00%) ⬇️
src/rw2image_int.hpp 0.00% <0.00%> (-100.00%) ⬇️
include/exiv2/exif.hpp 0.00% <0.00%> (-100.00%) ⬇️
include/exiv2/iptc.hpp 0.00% <0.00%> (-100.00%) ⬇️
include/exiv2/tags.hpp 0.00% <0.00%> (-100.00%) ⬇️
... and 80 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8505f4d...8ac0be3. Read the comment docs.

Copy link
Collaborator

@piponazo piponazo left a comment

Choose a reason for hiding this comment

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

I do not use much of the functionality of the exiv2 command line application and I am not the best to judge if that "feature" might have been used by somebody. IMHO it looks like a reasonable thing to limit 👍

Do you really need to add a new .jpg image for the tests? Could you just reuse some other .jpg file from the test/datafolder?

@postscript-dev
Copy link
Collaborator Author

@piponazo:

I do not use much of the functionality of the exiv2 command line application and I am not the best to judge if that "feature" might have been used by somebody. IMHO it looks like a reasonable thing to limit 👍

Previously in the exiv2 sample, I removed --verbose when using --extract with stdout. This prevented the filename from being prepended to the output, which would have corrupted the data. I realised afterward though that if multiple files were used, then all the output would be concatenated together and be corrupted.

Do you really need to add a new .jpg image for the tests? Could you just reuse some other .jpg file from the test/datafolder?

As for image files, I had wondered the same thing. I wasn't sure if it was better for each test to be self contained or if reducing the number of files was better. I am happy to follow the Exiv2 policy.

@piponazo
Copy link
Collaborator

piponazo commented Feb 4, 2022

As for image files, I had wondered the same thing. I wasn't sure if it was better for each test to be self contained or if reducing the number of files was better. I am happy to follow the Exiv2 policy.

I am not sure if we ever discussed about that ... I would say that it makes sense to add new files when it is the only way to check something new. If it is possible to check something with the existing test-data, I would not add more binaries into the repository.

@postscript-dev
Copy link
Collaborator Author

I am not sure if we ever discussed about that ... I would say that it makes sense to add new files when it is the only way to check something new. If it is possible to check something with the existing test-data, I would not add more binaries into the repository.

OK, I will switch the file to an existing one. After this merges, I will try and add a comment somewhere in the documentation to explain the policy.

@clanmills
Copy link
Collaborator

@postscript-dev You are the expert in the exiv2 command-line program. I'm sure everybody on Team Exiv2 agrees.

I thought there was something in tests/suite.conf to massage paths in the diff comparison. If all your input images are in $data_path, I believe the diff will succeed. Or perhaps I haven't understood your situation.

I really like how you used setUp()/open/read to deal with the reference output in #2058. Code can be beautiful. Good Job.

@postscript-dev
Copy link
Collaborator Author

@clanmills:

I thought there was something in tests/suite.conf to massage paths in the diff comparison. If all your input images are in $data_path, I believe the diff will succeed. Or perhaps I haven't understood your situation.

I really like how you used setUp()/open/read to deal with the reference output in #2058. Code can be beautiful. Good Job.

In both of your points, I had a problem with variables. I tried to use something like:

stdin = open("$filename", 'rb').read()

but it seems that open() is evaluated differently and the variables defined in the test suite and individual tests are not available. In the end, tests/README-TESTS.md was very helpful and provided the setup() answer. I don't know much about Python.

@postscript-dev postscript-dev marked this pull request as ready for review February 4, 2022 17:35
@clanmills
Copy link
Collaborator

I did work on README-TESTS.md. It needs lots more work. Dan's an amazing guy and engineer. For sure, he'll never win a Nobel prize for writing user documentation in English (and neither will I).

If there are architecture/structure issues with the test suite and tests/suite.conf, it would good to address them. @1div0 (Peter K) has offered to get more involved with testing.

@clanmills
Copy link
Collaborator

@postscript-dev Here's the PR for the work that I started on tests/README-TESTS.md #1553 I'm not asking you to finish this. I know you are already over-loaded. I'm amazed at the work involved in the Nikon/Flash business.

I suspect that the handling of environment variables in the test suite is hidden and should be exposed in the python API. For example, a typical test is test_pr_pr_1409.py and has the following code:

# -*- coding: utf-8 -*-

import system_tests

class FujiFilm_IFD_Tags_pr1409(metaclass=system_tests.CaseMeta):
    url = "https://github.com/Exiv2/exiv2/pull/1409"
    
    filename = "$data_path/exiv2-pr1409.exv"
    commands  = ["$exiv2 -g Fujifilm $filename"]
    stderr = [""]
    stdout = ["""Exif.Fujifilm.Version                        Undefined   4  48 49 51 48
Exif.Fujifilm.SerialNumber                   Ascii      48  FF0...
...
"""]
    retval = [0]

The variable filename = "$data_path/exiv2-pr1409.exv" is magically converted when the command "$exiv2 -g Fujifilm $filename" is executed by the framework. How can you use $data_path directly in your code? In your case you want to do something like:

stdout[0] = open('$data_path/1234.out','r').read()

Python has a library function os.path.expandvars(). The test suite should provide similar, so you can use:

stdout[0] = open(system_tests.expandvars('$data_path/1234.out'),'r').read()

It might even have this. I don't know. If you do the following in the terminal:

1142 rmills@rmillsmm-local:~/gnu/github/exiv2/0.27-maintenance/tests $ python3
Python 3.8.2 (default, Sep 24 2020, 19:37:08) 
[Clang 12.0.0 (clang-1200.0.32.21)] on darwin
Type "help", "copyright", "credits" or "license" for more information.
>>> import system_tests
>>> help(system_tests)

It reports an entry point expand_variables which is documented in README-TESTS.md as:

class SomeName(metaclass=system_tests.CaseMeta):

	def setUp(self):
		self.commands = [self.expand_variables("$some_var/foo.txt")]
		self.stderr = [""]
		self.stdout = [self.expand_variables("$success_message")]
		self.retval = [0]

And I guess that's exactly what you used in #2058.

@1div0 and @hassec Can you get involved with #1553 @postscript-dev has done a LOT of work in the last 12 months on the exiv2 man page and many other matters. We can't expect him to take on support, maintenance and development of the test suite.

@1div0
Copy link
Collaborator

1div0 commented Feb 5, 2022

@clanmills Yes, for sure I will take a 100k at #1553 and more as the time_t permits. But no ETA yet. Cheers. Best.

@postscript-dev postscript-dev merged commit 46c3290 into Exiv2:main Feb 5, 2022
@postscript-dev postscript-dev deleted the fix_exiv2_sample_verbose branch March 3, 2022 15:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants