Updating documentation on main to respect ctest #2034

clanmills · 2021-12-17T16:09:31Z

Thanks to a4865g for bring up this subject in #2031.

codecov · 2021-12-17T16:15:48Z

Codecov Report

Merging #2034 (14d7724) into main (29cc981) will not change coverage.
The diff coverage is n/a.

❗ Current head 14d7724 differs from pull request most recent head 10661d6. Consider uploading reports for the commit 10661d6 to get more accurate results

@@           Coverage Diff           @@
##             main    #2034   +/-   ##
=======================================
  Coverage   61.38%   61.38%           
=======================================
  Files          96       96           
  Lines       19214    19214           
  Branches     9852     9852           
=======================================
  Hits        11794    11794           
  Misses       5096     5096           
  Partials     2324     2324

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 29cc981...10661d6. Read the comment docs.

piponazo

As always, you took care of keeping the documentation up-to-date and put great attention to the details 👏

It looks great to me 👍

kmilos · 2021-12-18T11:37:19Z

CMakeLists.txt

@@ -118,9 +118,9 @@ if( EXIV2_BUILD_SAMPLES )
            WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/tests
            COMMAND cmake -E env EXIV2_BINDIR=${CMAKE_RUNTIME_OUTPUT_DIRECTORY} ${Python3_EXECUTABLE} runner.py --verbose tiff_test
        )
-        add_test(NAME lensTests


Why remove lensTest, they were running fine?

kmilos · 2021-12-18T11:39:19Z

CMakeLists.txt

+        add_test(NAME versionTest
            WORKING_DIRECTORY ${PROJECT_SOURCE_DIR}/tests
-            COMMAND cmake -E env EXIV2_BINDIR=${CMAKE_RUNTIME_OUTPUT_DIRECTORY} ${Python3_EXECUTABLE} runner.py --verbose lens_tests
+            COMMAND cmake -E env EXIV2_BINDIR=${CMAKE_RUNTIME_OUTPUT_DIRECTORY} ${Python3_EXECUTABLE} runner.py --verbose bash_tests/version_test.py 


Why is there a need for this, isn't version_test.py run as part of bashTests above already? I see below you wanted to have just the minimal versionTest w/o samples/unit tests (good idea), but then it should be addition, not removal of lensTests, no?

That's a mistake. I'll restore lensTest tomorrow.

I find the version test very useful. When I think "what have we got here", my finger rattle out $ make version_test. In future it'll be $ ctest -R version. In DOS, I have to type$ ctest -R version -C Release. The people at Kitware didn't think out the ctest -C option very well.

kmilos · 2021-12-18T11:40:04Z

README-CONAN.md

@@ -110,7 +110,7 @@ _Profiles for Visual Studio are discussed in detail here: [Visual Studio Notes](
 | _**1**_ | Get conan to fetch dependencies<br><br>The output can be quite<br>long as conan downloads and/or builds<br>zlib, expat, curl and other dependencies.| $ conan install ..<br>&nbsp;&nbsp;&nbsp;&nbsp;&nbsp;--build missing       | c:\\..\\build> conan install .. --build missing<br>&nbsp;&nbsp;&nbsp;&nbsp;--profile msvc2019Release64 |
 | _**2**_ | Get cmake to generate<br>makefiles or sln/vcxproj | $ cmake ..  | c:\\..\\build> cmake&nbsp;..&nbsp;-G&nbsp;"Visual Studio 16 2019"
 | _**3**_ | Build                                             | $ cmake --build .       | c:\\..\\build>&nbsp;cmake&nbsp;--build&nbsp;.&nbsp;--config&nbsp;Release<br>You may prefer to open exiv2.sln and build using the IDE. |
-| _**4**_ | Optionally Run Test Suite                         | $ make tests            | c:\\..\\build>&nbsp;cmake&nbsp;--build&nbsp;.&nbsp;--config&nbsp;Release --target tests<br/>[README.md](README.md) |
+| _**4**_ | Optionally Run Test Suite                         | $ make test             | c:\\..\\build>&nbsp;cmake&nbsp;--build&nbsp;.&nbsp;--config&nbsp;Release --target test<br/>[README.md](README.md) |


Aren't we removing "make test" everywhere in favor of ctest?

Yes. We are. It's work-in-progress.

kmilos · 2021-12-18T11:41:03Z

README.md

-$ make tests
-$ sudo make install
+$ ctest
+$ sudo cmake --build . --target install


Or even shorter/recommended cmake --install ., but six vs half a dozen really...

I'd prefer to keep the cmake --build . --target install version because that is consistent with everything else.

I didn't know about cmake --install .. That's getting more involved with cmake than necessary. It's much the same with conan and ctest, there are many options. I like to keep life simple and give the user the minimum required for success.

kmilos · 2021-12-18T11:46:19Z

README.md

 ... lots of output ...
+$ cmake --build . --target test


ctest instead?

I think I fixed that this morning on 0.27-maintenance. That's when I realised "there's still a lot of work to be done here. I'll focus on getting 0.27-maintenance correct. Then punt to 'main'.". So, then I made the 'main' PR a draft.

We were boozing last night. I'll run tomorrow morning, then I'll have fun fixing everything. It's our son's wedding anniversary today, so they've gone to Bath for the weekend.

kmilos · 2021-12-18T11:46:46Z

README.md


 ```bash
 > cd <exiv2dir>/build
-> cmake --build . --target tests
-> cmake --build . --target python_tests
+> cmake --build . --target test


README.md

kmilos · 2021-12-18T11:48:13Z

README.md

-
-The name **bash_tests** is historical.  They are implemented in python.
+|              | $ cd \<exiv2dir\>/build            | \> cd \<exiv2dir\>/build              |
+| test         | $ ctest                            | \> cmake --build . --config Release --target test         |


The --config Release is now redundant everywhere since we made it default.

I need to carefully consider and document cmake --config option.

clanmills · 2021-12-18T12:08:41Z

I should not have removed lensTest on 'main'. lensTest on 0.27-maintenance is broken was broken and effectively ran all the tests for a second time. I'll put it back on 'main' tomorrow and investigate adding it to 0.27-maintenance.

I've marked both PRs (for 'main' and '0.27-maintence') as "Draft". I'm having a day off (hell, it's Saturday). I'll deal with all your comments tomorrow. My aim is tiny differences between the two branches will (or even no difference at all).

@piponazo Thank You for your words of praise and encouragement. I consider these documents to be very important. It's time well spent to ensure they are accurate and comprehensive. I'm very pleased by the effort that @postscript-dev put into the exiv2 man page.

…and 0.27-maintenance/README.md.

kmilos

Just a couple of typos Robin

README.md

clanmills · 2021-12-20T11:50:52Z

I've fixed the $ $ typo on 0.27-maintenance. When the CI is green (other that the insane old woman codecov), I'll merge both 0.27-maintenance and main.

This has been well worth the effort and good team-work.

Happy Holidays Everybody.

Updating documentation to respect ctest.

122fb95

clanmills requested review from piponazo and kevinbackhouse December 17, 2021 16:09

clanmills mentioned this pull request Dec 17, 2021

Fix wrong make test #2031

Closed

clanmills changed the title ~~Updating documentation to respect ctest.~~ Updating documentation on main to respect ctest Dec 17, 2021

clanmills marked this pull request as draft December 18, 2021 10:00

piponazo previously approved these changes Dec 18, 2021

View reviewed changes

kmilos reviewed Dec 18, 2021

View reviewed changes

Restored test lensTest. Minimized differences between main/README.md …

5693979

…and 0.27-maintenance/README.md.

clanmills dismissed piponazo’s stale review via 5693979 December 19, 2021 12:02

clanmills marked this pull request as ready for review December 19, 2021 12:05

Adding version_test.py.

82371c4

clanmills requested review from piponazo and kmilos December 19, 2021 22:05

Fixes following review.

e32d3fc

kmilos requested changes Dec 20, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

README.md Outdated Show resolved Hide resolved

kmilos reviewed Dec 20, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Fixes following review.

f3393f8

kmilos reviewed Dec 20, 2021

View reviewed changes

README.md Outdated Show resolved Hide resolved

Fix $ $ typo. Well spotted, @kmilos.

10661d6

kmilos approved these changes Dec 20, 2021

View reviewed changes

clanmills merged commit 41ae517 into main Dec 20, 2021

clanmills deleted the fix_README_ctest_main branch December 20, 2021 12:10

kmilos added this to the v1.00 milestone Dec 22, 2021

nehaljwani mentioned this pull request Apr 30, 2023

Is a new release happening soon? #2406

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Updating documentation on main to respect ctest #2034

Updating documentation on main to respect ctest #2034

clanmills commented Dec 17, 2021

codecov bot commented Dec 17, 2021 •

edited

Loading

piponazo left a comment

kmilos Dec 18, 2021

kmilos Dec 18, 2021

clanmills Dec 18, 2021

kmilos Dec 18, 2021

clanmills Dec 18, 2021

kmilos Dec 18, 2021

clanmills Dec 18, 2021

kmilos Dec 18, 2021

clanmills Dec 18, 2021

kmilos Dec 18, 2021

kmilos Dec 18, 2021

kmilos Dec 18, 2021

clanmills Dec 18, 2021

clanmills commented Dec 18, 2021

kmilos left a comment

clanmills commented Dec 20, 2021

Updating documentation on main to respect ctest #2034

Updating documentation on main to respect ctest #2034

Conversation

clanmills commented Dec 17, 2021

codecov bot commented Dec 17, 2021 • edited Loading

Codecov Report

piponazo left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

clanmills commented Dec 18, 2021

kmilos left a comment

Choose a reason for hiding this comment

clanmills commented Dec 20, 2021

codecov bot commented Dec 17, 2021 •

edited

Loading