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 pr#2053 for native Windows builds #2058

Merged

Conversation

postscript-dev
Copy link
Collaborator

@postscript-dev postscript-dev commented Jan 19, 2022

Fixes:

  1. Fix Windows native build error introduced in pr#2053. Correct problem of mismatched Windows/Linux newline characters.
  2. Remove dependency on command line tools being available.

@postscript-dev postscript-dev added bug platforms Cygwin, MinGW, cross-compilation, NetBSD, FreeBSD etc testing Anything related to the tests and their infrastructure labels Jan 19, 2022
@postscript-dev postscript-dev added this to the v1.00 milestone Jan 19, 2022
@postscript-dev postscript-dev self-assigned this Jan 19, 2022
@codecov
Copy link

codecov bot commented Jan 19, 2022

Codecov Report

Merging #2058 (44ebb2f) into main (e56abfa) will decrease coverage by 55.76%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##             main   #2058       +/-   ##
==========================================
- Coverage   61.48%   5.72%   -55.77%     
==========================================
  Files          96      96               
  Lines       19207   18955      -252     
  Branches     9843    9843               
==========================================
- Hits        11810    1085    -10725     
- Misses       5080   17682    +12602     
+ Partials     2317     188     -2129     
Impacted Files Coverage Δ
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%) ⬇️
src/tiffvisitor_int.hpp 0.00% <0.00%> (-100.00%) ⬇️
... and 79 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 e56abfa...44ebb2f. Read the comment docs.

@kevinbackhouse
Copy link
Collaborator

For future reference: this is a relatively new test, added in #2053.

@kevinbackhouse
Copy link
Collaborator

@postscript-dev: it looks like this test never caused a problem in our automated Windows tests. Do you have any idea what's different about the automated tests that stopped them from being affected?

@postscript-dev
Copy link
Collaborator Author

Sorry, I don't know. I use MSYS2/MinGW64 and the output uses Windows newlines. The temp file is:
issue_1959_poc.xmp_save.out.zip.

@postscript-dev
Copy link
Collaborator Author

postscript-dev commented Jan 19, 2022 via email

@kevinbackhouse
Copy link
Collaborator

I was wondering if this could be caused by git auto-converting the line endings when you check the code out. Which file has the Windows line endings: $filename_save or $filename_out?

@clanmills
Copy link
Collaborator

clanmills commented Jan 21, 2022

@postscript-dev I'm surprised you're executing diff/cmp. The usual workflow in the test suite is to store the "reference output" in the script (in the stdout array) and allow the script to output. The framework will perform the diff for you. By keeping everything inside Dan's excellent python framework, you avoid platform ugliness to ensure utilities such as diff/cmp are on the PATH.

@postscript-dev
Copy link
Collaborator Author

@kevinbackhouse: Sorry for the delay. In MSYS2/MinGW64, I deleted the filename, filename_save and filename_out files, then restored them using git and ran the Python test again. filename and filename_save both contained the Linux newlines (which is the same as in git) and filename_out (which was generated by the Python test) has the Windows newlines. Restoring the files using git didn't change the endings on my machine.

I don't have Visual Studio installed but in Windows 11, I installed Python3 and downloaded the latest official Windows nightly build (exiv2-1.0.0.9-2019msvc64.zip). When I ran tests\bugfixes\github\test_issue_1959.py, this behaved the same as the MSYS2/MinGW version. Both filename_out files have Windows newlines.

@clanmills: You are right about using the Python compare and I did consider that method. As I included a lot of tags in the file, the output is quite long and I was unsure about adding that much text to a Python test. Your comment about not having cmp or diff in the path, is interesting though. Do you think that I should change this to use the Python stdout array?

@clanmills
Copy link
Collaborator

clanmills commented Jan 21, 2022

@postscript-dev I think you should leave the test framework to do the comparison for you.

If the "reference output" is really long, you can probably read it into the test suite. Typical test code is:

       stdin    = [ "dkdkdkdkd", "kekeke010101" ]
       stderr   = [ "",  "" ]

Instead of having huge strings in the python code, you could read them from a file. Something like this (take care, my python's rusty and probably wrong) but you get the idea:

       stderr   = [ "", "" ]
       stdin    = [ "", "" ]
       stdin[0] = open('reference_1234_0.txt', 'rb').read()
       stdin[1] = open('reference_1234_1.txt', 'rb').read()

There are variables set from tests/suite.conf to identify directories. For example: data_path is the path to the test/data. You might need to use python's os.path.join. I don't remember much about the test suite, however it is well designed.

@postscript-dev
Copy link
Collaborator Author

@clanmills: Thanks for explaining, I will change the test over to use the Python framework instead. This will be a job for next week sometime.

@postscript-dev postscript-dev marked this pull request as draft January 22, 2022 17:03
Both Windows and Linux/macOS use different newline characters in
output.

In native Windows builds, Python test fails when using `cmp` to
compare files. Use `diff --strip-trailing-cr` instead.

The Windows CI builds were unaffected by the problem.
Removes dependency on command line tools being available.
@postscript-dev postscript-dev marked this pull request as ready for review February 4, 2022 12:07
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.

LGTM! Thanks!

@postscript-dev postscript-dev merged commit 8505f4d into Exiv2:main Feb 4, 2022
@postscript-dev postscript-dev deleted the fix_pr#2053_native_Win_newlines branch February 4, 2022 16:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug platforms Cygwin, MinGW, cross-compilation, NetBSD, FreeBSD etc testing Anything related to the tests and their infrastructure
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants