Robin's Changes between RC2 and RC3 #557

clanmills · 2018-11-21T08:40:04Z

I'm rather surprised that conversions.sh reveals a (harmless) order change on:

runTest exiv2 -M'set Xmp.xmp.ModifyDate 2015-04-17T18:10:22Z' v.xmp

No other part of the test suite notices any change and the order of the Xmp keys is not important.

I have stepped the code in the debugger and the key is added in xmp.cpp:

    int XmpData::add(const Xmpdatum& xmpDatum)
    {
        xmpMetadata_.push_back(xmpDatum);
        return 0;
    }

codecov · 2018-11-21T10:03:21Z

Codecov Report

Merging #557 into master will increase coverage by 0.03%.
The diff coverage is 100%.

@@            Coverage Diff             @@
##           master     #557      +/-   ##
==========================================
+ Coverage   62.74%   62.77%   +0.03%     
==========================================
  Files         154      154              
  Lines       20680    20695      +15     
==========================================
+ Hits        12976    12992      +16     
+ Misses       7704     7703       -1

Impacted Files	Coverage Δ
include/exiv2/xmp_exiv2.hpp	`100% <ø> (ø)`	⬆️
src/xmp.cpp	`90.64% <100%> (+0.25%)`	⬆️
src/version.cpp	`90.79% <100%> (+0.35%)`	⬆️
src/actions.cpp	`74.15% <100%> (+0.08%)`	⬆️

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 0d7b3ec...4016fe0. Read the comment docs.

clanmills · 2018-11-21T10:45:33Z

I'm going to keep working on this until I understand exactly what happened to conversions.sh. And I want to add Gilles Test file DSC_3079.jpg to the test suite.

In convert.cpp there is a call to xmpData_->erase(pos);. This is the function which I modified. Clearly there's a side-effect which seems to be benign. More work needed.

I have added a new function XmpData::erase1() which has the "old" implementation and use xmpData_->erase1(pos) in convert.cpp. That's fine now. Clearly there's a functional difference between erase() and erase1() which should be fixed.

    bool Converter::prepareXmpTarget(const char* to, bool force)
    {
        Exiv2::XmpData::iterator pos = xmpData_->findKey(XmpKey(to));
        if (pos == xmpData_->end()) return true;
        if (!overwrite_ && !force) return false;
        xmpData_->erase(pos);
        return true;
    }

I've reproduced this behaviour with a simpler script acon.sh

#!/usr/bin/env bash

##
# generate the file u.xmp from "empty"
cp    ../test/data/exiv2-empty.jpg                                       u.jpg
bin/exiv2 -M'set Exif.Photo.DateTimeOriginal 2003:12:14 12:01:44'        u.jpg
bin/exiv2 -M'set Exif.Photo.SubSecTimeOriginal 999999999'                u.jpg
bin/exiv2 -M'set Exif.Photo.ExifVersion 48 50 50 49'                     u.jpg
bin/exiv2 -M'set Exif.Photo.ComponentsConfiguration 1 2 3 0'             u.jpg
bin/exiv2 -M'set Exif.Photo.Flash 73'                                    u.jpg
bin/exiv2 -M'set Exif.GPSInfo.GPSLatitude 3/1 8/1 29734512/1000000'      u.jpg
bin/exiv2 -M'set Exif.GPSInfo.GPSLatitudeRef N'                          u.jpg
bin/exiv2 -M'set Exif.GPSInfo.GPSVersionID 2 2 0 1'                      u.jpg
bin/exiv2 -M'set Exif.GPSInfo.GPSTimeStamp 1/1 2/1 999999999/1000000000' u.jpg
bin/exiv2 -eX --verbose --force u.jpg

##
# copy and update it
cp  u.xmp                                                 v.xmp
bin/exiv2 -M'set Xmp.xmp.ModifyDate 2015-04-17T18:10:22Z' v.xmp
bin/exiv2 -px                                             v.xmp

# That's all Folks!
##

Original behaviour:

677 rmills@rmillsmm:~/gnu/github/exiv2/master/build $ ../../exiv2/cmake-build-debug/acon.sh 
File 1/1: u.jpg
Writing Exif data from u.jpg to ./u.xmp
Xmp.exif.ExifVersion                         XmpText     4  2.21
Xmp.exif.GPSVersionID                        XmpText     7  2.2.0.1
Xmp.exif.GPSLatitude                         XmpText    12  3,8.4955752N
Xmp.exif.GPSTimeStamp                        XmpText    29  2003-12-14T01:02:00.999999999
Xmp.exif.Flash                               XmpText     0  type="Struct"
Xmp.exif.Flash/exif:Fired                    XmpText     4  True
Xmp.exif.Flash/exif:Return                   XmpText     1  0
Xmp.exif.Flash/exif:Mode                     XmpText     1  1
Xmp.exif.Flash/exif:Function                 XmpText     5  False
Xmp.exif.Flash/exif:RedEyeMode               XmpText     4  True
Xmp.exif.ComponentsConfiguration             XmpSeq      4  YCbCr
Xmp.xmp.ModifyDate                           XmpText    20  2015-04-17T18:10:22Z <-- He's almost last
Xmp.photoshop.DateCreated                    XmpText    29  2003-12-14T12:01:44.999999999

After my change:

678 rmills@rmillsmm:~/gnu/github/exiv2/master/build $ cd ../../exiv2/cmake-build-debug/
679 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/cmake-build-debug $ ../../exiv2/cmake-build-debug/acon.sh 
File 1/1: u.jpg
Writing Exif data from u.jpg to ./u.xmp
Xmp.xmp.ModifyDate                           XmpText    20  2015-04-17T18:10:22Z  <--- He's first
Xmp.exif.ExifVersion                         XmpText     4  2.21
Xmp.exif.GPSVersionID                        XmpText     7  2.2.0.1
Xmp.exif.GPSLatitude                         XmpText    12  3,8.4955752N
Xmp.exif.GPSTimeStamp                        XmpText    29  2003-12-14T01:02:00.999999999
Xmp.exif.ComponentsConfiguration             XmpSeq      4  YCbCr
Xmp.exif.Flash                               XmpText     0  type="Struct"
Xmp.exif.Flash/exif:Fired                    XmpText     4  True
Xmp.exif.Flash/exif:Return                   XmpText     1  0
Xmp.exif.Flash/exif:Mode                     XmpText     1  1
Xmp.exif.Flash/exif:Function                 XmpText     5  False
Xmp.exif.Flash/exif:RedEyeMode               XmpText     4  True
Xmp.photoshop.DateCreated                    XmpText    29  2003-12-14T12:01:44.999999999
680 rmills@rmillsmm:~/gnu/github/exiv2/exiv2/cmake-build-debug $

clanmills · 2018-11-21T17:03:07Z

I have dug over this carefully. I believe the earlier version of this is safe and arrives at the correct result. However, the test suite detected a difference.

I restored XmpDatum.erase() to its previous behaviour of deleting the item at 'pos'. I added a new API XmpDatum.eraseFamily() which deletes the item and its children. Also add Gilles test file DSC_3079.jpg to the test suite and added a test in conversions.sh.

I think the earlier version of the code in which I modified XmpDatum.erase() to perform its previous role AND eraseFamily() is superior and logical. However, as we are close to the end of Exiv2 v0.27, I feel there is less risk by adding XmpDatum.eraseFamily(). actions.cpp has been modified to call eraseFamily().

piponazo

The changes look good to me.

I would have preferred to see the new tests in the python suite instead of adding them in the bash tests suite (test/conversions.sh). However, I understand that you are not as familiar with the python suite as with the bash one. We can convert the remaining bash scripts after releasing v0.27.

Is there any chance of reducing further the size of the binary file (image) by extracting the metadata into a .exv file ?

piponazo · 2018-11-28T13:48:34Z

src/CMakeLists.txt

    endif()

    target_link_libraries( exiv2lib PRIVATE Threads::Threads)
+else()
+    target_link_libraries( exiv2lib PRIVATE psapi ws2_32 )


Now we have target_link_libraries( exiv2lib PRIVATE psapi ws2_32 ) in two different places (when MSVC or when CYGWIN OR MINGW OR MSYS). I think that it would be simpler to just have it once when WIN32.

We could reuse for that the already existing conditional in line 184.

clanmills · 2018-11-28T14:15:31Z

Yes. You are right. This can be simplified.

When I was looking at the code I thought "oh that's odd, it doesn't seem to do this for MSVC. So how could it ever have worked? I'll try explicitly adding it and see what happens!". We now know how it works. When building exiv2.dll, the linker does not worry about unsatisfied externals. When building (static) exiv2.lib, psapi must be linked at build-time.

You're right. Much better to do it for WIN32 as this has nothing really to do with MSVC. And it has nothing really to do with the build-type either (static/shared).

piponazo · 2018-11-28T17:11:49Z

cmake/packaging.cmake

-    set(CPACK_GENERATOR ZIP)  # use .zip - less likely to damage bin/exiv2.dll permissions
-else()
-    set(CPACK_GENERATOR TGZ)  # MinGW/Cygwin/Linux/MacOS-X etc use .tar.gz
+set (CC "") # Compiler


Few comments here.

The default value of CC is empty. Is that the intended behaviour? In case the compiler is not clang, the value will remain empty.

The way to check for clang can be simplified (without creating variables for it):

if (${CMAKE_CXX_COMPILER_ID} MATCHES "Clang") set (CC Clang) endif()

Yes. That's what I want. We'll end up with bundle names such as:

CYGWIN64ClangStaticDebug

However, if for a SharedRelease built with GCC, the name will simply be:

CYGWIN64

I think there are 60 common build possibilities.
{Linux|CYGWIN|MinGW}{64|32}{''|Clang}{''|Static}{''|Debug} = 3x2x2x2x2 = 48
{Darwin}{''|Static}{''|Debug} = 2x2 = 4
{msvc}{64|32}{''|Static}{''|Debug} = 8
Total = 60

If I decide to to build with the other versions of Visual Studio {2008|2010|2012|2013|2015), that will be 5x8=40 more builds.

If we have a FULL build (webready, video) that would double it to 200.

There's no problem presenting and storing this on the buildserver. http://exiv2.dyndns.org:8080/userContent/builds/Platform/

On the downloads page, we'll only offer 6 http://exiv2.dyndns.org/download.html
Source
msvc64
Linux64
Darwin
MinGW64
CYGWIN64

For RC1 and RC2, I published MinGW64 and MinGW32 on 'downloads' because we were experiencing different issues on those platforms. However, we've put that behind us now, so I'll only offer MinGW64 with RC3.

D4N · 2018-11-28T22:30:45Z

README.md

+$ make
+```
+
+Due to the way in which ccache is installed in Fedora, this option is defeated on that platform.  [https://github.com/Exiv2/exiv2/issues/361](https://github.com/Exiv2/exiv2/issues/361)


This sounds like ccache does not work on Fedora, which is no true. This option just can't turn it off, that's all.

Ah @D4N Thanks for the clarification. As I said in #361 "This is very confusing.". I'll update the documentation tomorrow.

@piponazo I've changed the code in cmake/packaging.cmake that detects clang.

piponazo · 2018-11-29T07:18:24Z

@clanmills since this PR is going to merge 0.27-RC3 into master, which is fixing many issues, could you change the description of the PR and create a list of the issues which you have fixed in the branch ? Right now, if you look at the PR description it looks like it is just fixing one issue.

clanmills · 2018-11-29T08:11:04Z

Sure. When I get to the end of this marathon, I'll update releasenotes/releasenotes.txt with a list of everything changed since RC2. It will include:

Updates to documentation concerning Debugging, Clang and ccache.
Build and test logs are stored in the build bundles.
Build Server publishes and tests Cygwin, MinGW, Linux, Darwin and msvc in Release/Debug, 64/32, gcc/clang, static/shared.
Revised man page exiv2.1.
Fix del XMP/Region (How to remove XMP Region tag from JPEG ? #521)
All compiler warnings have been eliminated.
Web-site has been refined and polished.
Preparation to rehost exiv2.org
More security fixes (AddressSanitizer: heap-buffer-overflow at types.cpp:245 #425, Insufficient verification(cycle) in function Image::printIFDStructure in image.cpp:335 #547, Disable -pR option in Release mode #558, Fix in isoSpeed() #564)
Something I've forgotten!
Something we'll discover today or tomorrow!

We will reach RC3 tomorrow (Friday 30 November). Any security fixes next week will be added and RC3 will be published on Friday 7 December. I'm hoping for no changes between RC3 and GM.

If we have "dot" releases, every quarter, security fixes will go into 0.27.1 at the end of March 2019.

conversions.sh reveals a (harmless) order change runTest exiv2 -M'set Xmp.xmp.ModifyDate 2015-04-17T18:10:22Z' v.xmp

…eFamily(). Restored and augumented conversions.sh

…h conversions.sh

…/windows/desktop/Services/installing-a-service

…s GetModuleFileName isn't effective!

- Consuming with CMake - Debugging Exiv2 - Added "2.13 Building Exiv2 with ccache". - Revised "Building Exiv2 with ccache" in response to Dan's feedback. - Fixed typo in README.md - ReleaseNotes update in preparation for RC3.

clanmills assigned piponazo Nov 21, 2018

clanmills assigned clanmills and unassigned piponazo Nov 21, 2018

clanmills requested a review from piponazo November 21, 2018 16:53

piponazo approved these changes Nov 21, 2018

View reviewed changes

clanmills mentioned this pull request Nov 22, 2018

MSVC debug build issues #560

Closed

piponazo force-pushed the 0.27-RC3 branch from 72ba396 to 61ba40a Compare November 26, 2018 15:41

piponazo requested changes Nov 28, 2018

View reviewed changes

piponazo reviewed Nov 28, 2018

View reviewed changes

D4N reviewed Nov 28, 2018

View reviewed changes

clanmills changed the title ~~Fix del XMP/Region (#521)~~ Robin's Changes between RC2 and RC3 Nov 29, 2018

clanmills added this to the v0.27 milestone Nov 29, 2018

clanmills added 12 commits November 29, 2018 16:49

Fix: #521

b8d4477

conversions.sh reveals a (harmless) order change runTest exiv2 -M'set Xmp.xmp.ModifyDate 2015-04-17T18:10:22Z' v.xmp

Revision to PR#557. Restored XmpData::erase() and added XmpData::eras…

b813234

…eFamily(). Restored and augumented conversions.sh

Bump Version to 0.27.0.3

2c3e1f6

Adding output for have_regex to version.cpp

e4104d2

Fix for #560 (eraseFamily() crashes in MSVC/Debug).

2646369

Add cmake/msvc_conan_profiles

a26535a

src/version.cpp report enable_nls

b376975

Add packaging changes for CYGWIN64/CYGWIN32 and Linux32

85dc2c1

add logs to the build bundle

9afd535

Update platform/ReadMe.txt about logs.

1a69169

#485 Tweak msvc output to avoid the test suite reporting an issue wit…

52e51b7

…h conversions.sh

Fix: #571

a3cd401

clanmills added 11 commits November 29, 2018 16:59

Fix: #570

ede4b8e

Change package names Platform{64|32}[Static][Debug]

5888477

packaging cleanup

2e17f2e

Try to fix #572

103b182

Correction in futils.cpp

d1e3f47

Correction to Microsoft sample code! https://docs.microsoft.com/en-gb…

948c48d

…/windows/desktop/Services/installing-a-service

#571 Restore the original windows getProcessCode() because Microsoft'…

8980eac

…s GetModuleFileName isn't effective!

Updating packaging for clang: Platform[{64|32}][Clang][Static][Debug]

58e7bfb

Correction to previous commit.

d5aa726

Documentation updates + release notes

bd9de9b

- Consuming with CMake - Debugging Exiv2 - Added "2.13 Building Exiv2 with ccache". - Revised "Building Exiv2 with ccache" in response to Dan's feedback. - Fixed typo in README.md - ReleaseNotes update in preparation for RC3.

Change how packaging detects Clang.

62ca7c0

piponazo force-pushed the 0.27-RC3 branch from 0395bf3 to 62ca7c0 Compare November 29, 2018 16:00

piponazo approved these changes Nov 29, 2018

View reviewed changes

This was referenced Nov 29, 2018

Ugly compiler warning on MinGW/msys2 from src/localhost.c #571

Closed

MSVC iotest.cpp link failure #572

Closed

Ugly compiler warnings on MacOS-X from xmpsdk/src/XMPMeta.cpp #570

Closed

DBUILD_WITH_CCACHE option does not work on Fedora #361

Closed

clanmills added 2 commits November 29, 2018 19:10

Restore cpack_source_generator

25a3de8

Do not put "Clang" in the Darwin build bundle name.

4016fe0

piponazo merged commit d8a15e1 into master Nov 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Robin's Changes between RC2 and RC3 #557

Robin's Changes between RC2 and RC3 #557

clanmills commented Nov 21, 2018

codecov bot commented Nov 21, 2018 •

edited

Loading

clanmills commented Nov 21, 2018 •

edited

Loading

clanmills commented Nov 21, 2018

piponazo left a comment

piponazo Nov 28, 2018

piponazo Nov 28, 2018

clanmills commented Nov 28, 2018

piponazo Nov 28, 2018

clanmills Nov 28, 2018

D4N Nov 28, 2018

clanmills Nov 28, 2018

clanmills Nov 29, 2018

piponazo commented Nov 29, 2018

clanmills commented Nov 29, 2018

Robin's Changes between RC2 and RC3 #557

Robin's Changes between RC2 and RC3 #557

Conversation

clanmills commented Nov 21, 2018

codecov bot commented Nov 21, 2018 • edited Loading

Codecov Report

clanmills commented Nov 21, 2018 • edited Loading

clanmills commented Nov 21, 2018

piponazo left a comment

Choose a reason for hiding this comment

piponazo Nov 28, 2018

Choose a reason for hiding this comment

piponazo Nov 28, 2018

Choose a reason for hiding this comment

clanmills commented Nov 28, 2018

piponazo Nov 28, 2018

Choose a reason for hiding this comment

clanmills Nov 28, 2018

Choose a reason for hiding this comment

D4N Nov 28, 2018

Choose a reason for hiding this comment

clanmills Nov 28, 2018

Choose a reason for hiding this comment

clanmills Nov 29, 2018

Choose a reason for hiding this comment

piponazo commented Nov 29, 2018

clanmills commented Nov 29, 2018

codecov bot commented Nov 21, 2018 •

edited

Loading

clanmills commented Nov 21, 2018 •

edited

Loading