Seems we'll need a find_package(fmt) in the STIRConfig.cmake.in, but this is of course rather undesirable, as now we have an extra external dependency anyway for anyone who wants to use STIR. Far better would be to make fmt a private dependency. I think this would work if we don't use it in src/include/stir/FactoryRegistry.inl.

CMake targets are useful, but a pain...

I have no idea why still fails after including it in STIRConfig.cmake. I see that the log doesn't say it found fmt (while it does say it found boost). A bug in fmt?

Potentially we could

• use fmt's FMT_HEADER_ONLY and use the work-around from

  STIR/src/IO/CMakeLists.txt

  Lines 121 to 129 in 4e57186

  	# target_link_libraries(IO PUBLIC PRIVATE "$<BUILD_INTERFACE:nlohmann_json::nlohmann_json>")
  	# So, we currently use an ugly work-around from
  	# https://gitlab.kitware.com/cmake/cmake/-/issues/15415#note_334852
  	# Warning: this will fail once nlohman_json stops being header-only!
  	# In that case, we will need to add it in STIRConfig.cmake.in
  	#
  	get_target_property(TMP nlohmann_json::nlohmann_json INTERFACE_INCLUDE_DIRECTORIES)
  	target_include_directories(IO PRIVATE "${TMP}")

  (although this could be somewhat outdated according to the CMake issue). This work-around is ugly of course, and as we'd need to repeat it many times, maybe it needs a macro.
• somehow make the target part of the STIRConfig.cmake.

One more of these very simple ideas that turns into a nightmare. Sigh.

yes, apt installing libfmt was definitely easier than battling with cmake 😅 I'll play around a bit longer, but maybe switching to header only is the most promising

Calling find_package(fmt) earlier on in STIRConfig.cmake.in fixed the issue! Now an installed STIR can be used without any changes.

Not sure what the Codacy issue is about - do you have any experience with this @KrisThielemans ?

Also, I seem to have gotten appveyor into a weird state :-/

The clang-tidy printf and fprintf replacement is a bit underwhelming so far - only picked up a few instances and skipped many others (maybe because I don't build some code). But it also turns out that it doesn't replace sprintf.

Not sure what the Codacy issue is about

it complains about

return internal_format::format(fmt, std::forward<Args>(args)...);

with "Comparison of a boolean expression with an integer." Doesn't have anything to do with us, so let's ignore it.

Also, I seem to have gotten appveyor into a weird state :-/

It reports

CMake Error at CMakeLists.txt:123 (message):
  The {fmt} submodule was not downloaded! GIT_SUBMODULE was turned off or
  failed.  Please update submodules and try again.

this seems to say that your attempt to auto-update the module fails. Not sure why... I'm not sure if we should do this though. I think I'd be surprised if building my software suddenly updates submodules. I think I'd prefer to give explicit instructions how to clone, as well as an error message that tells them what to do (i.e. either use C++-20, or use the explicit git command). In that case, we'll have to modify appveyor.yml. https://www.appveyor.com/docs/how-to/private-git-sub-modules/ suggests to add

install:
  - git submodule update --init --recursive

appveyor/ci#899 claims this doesn't work well with caching and gives a far more complicated solution. Maybe we can ignore that for a while.

Also, ideally we keep the clang-tidy changes in a separate commit, which we then add to .git-blame-ignore-revs. sorry

Also, I seem to have gotten appveyor into a weird state :-/

It reports

CMake Error at CMakeLists.txt:123 (message):
  The {fmt} submodule was not downloaded! GIT_SUBMODULE was turned off or
  failed.  Please update submodules and try again.

this seems to say that your attempt to auto-update the module fails. Not sure why... I'm not sure if we should do this though. I think I'd be surprised if building my software suddenly updates submodules. I think I'd prefer to give explicit instructions how to clone, as well as an error message that tells them what to do (i.e. either use C++-20, or use the explicit git command). In that case, we'll have to modify appveyor.yml. https://www.appveyor.com/docs/how-to/private-git-sub-modules/ suggests to add

install:
  - git submodule update --init --recursive

appveyor/ci#899 claims this doesn't work well with caching and gives a far more complicated solution. Maybe we can ignore that for a while.

Unfortunately, this does not work because we currently do a shallow clone of the repository:
https://www.appveyor.com/docs/how-to/repository-shallow-clone/

Do you think we could switch to specifying a clone depth?

Do you think we could switch to specifying a clone depth?

sure

Even with STIR master I get the following error when I run "./run_tests_modelling.sh" from within STIR/recon_test_pack. Am I doing something wrong or is this test currently broken?

Using executables like the following
Using /workspace/stir-public/deploy/bin/get_dynamic_images_from_parametric_images
Using /workspace/stir-public/deploy/bin/forward_project

Maximum absolute error = 0
Maximum in (1st - 2nd) = 0
Minimum in (1st - 2nd) = 0
Error relative to sup-norm of first image = 0 %

Image arrays are identical
(tolerance used: 0.05 %)


Maximum absolute error = 0
Maximum in (1st - 2nd) = 0
Minimum in (1st - 2nd) = 0
Error relative to sup-norm of first image = 0 %

Image arrays are identical
(tolerance used: 0.05 %)


ERROR: First line of input function file (plasma.if) is not number of samples
terminate called after throwing an instance of 'std::runtime_error'
  what():  
ERROR: First line of input function file (plasma.if) is not number of samples

./run_tests_modelling.sh: line 143: 36557 Aborted                 (core dumped) get_dynamic_images_from_parametric_images dyn_from_p0005-p5.hv p0005-p5.${imgext} ${INPUTDIR}PatlakPlot.par
ERROR

weird. it works for me. Indeed, it works in the Actions. see for instance
https://github.com/UCL/STIR/actions/runs/14836231076/job/41648053623?pr=1591#step:17:1945

Bad news: fmt definitely doesn't support the "%d" syntax, so for input files we'll have to revert to boost if we want to be backward compatible.

Also, I managed to debug into the problematic line in run_tests_modelling.sh and the problem is the "if(*p)". I notice that in my case the character array for the first line shows as "3655\r", so *p interprets to "\r". Turned out to be caused by line endings: my file had CRLF line endings, and when changing the line endings for plasma.if to LF, then reading the file works fine. Afterwards I also had to change the "for fr in count 23 28"-style loops to "for fr in $(seq 23 28)", since count does not seem to be supported in ubuntu. Now it runs through fine.

Bad news: fmt definitely doesn't support the "%d" syntax, so for input files we'll have to revert to boost if we want to be backward compatible.

sigh. what about the following then: if the string contains a %, write a warning recommending to use the new syntax and fall-back to boost::format, otherwise use fmt. That will allow us to deprecate it later. If you have linking problems with boost::format, we could do an "replace %d with {} and hope for the best".

Also, I managed to debug into the problematic line in run_tests_modelling.sh and the problem is the "if(*p)". I notice that in my case the character array for the first line shows as "3655\r", so *p interprets to "\r". Turned out to be caused by line endings: my file had CRLF line endings, and when changing the line endings for plasma.if to LF, then reading the file works fine.

That's pretty weird. and git status shows no change in the file? what does the following say?

git check-attr -a -- test_modelling_input/plasma.if

(in my case: nothing).

I checked that on Ubuntu, my file has Unix EOL (i.e. \n), while on Windows it has DOS EOL (i.e. \r \n). Somehow you have a mixture. Maybe this is affected by a git switch for preference?

Can you check another text file, e.g.

$ od -c test_modelling_input/ECAT_931_projdata_template.hs|head
0000000   !   I   N   T   E   R   F   I   L   E           :   =  \n   n
0000020   a   m   e       o   f       d   a   t   a       f   i   l   e
...

Of course, ideally we'd rewrite the code to cope with "DOS EOL on Unix" (KeyParser does this), but I doubt you feel like doing that!

Afterwards I also had to change the "for fr in count 23 28"-style loops to "for fr in $(seq 23 28)", since count does not seem to be supported in ubuntu. Now it runs through fine.

count is a STIR script that should have been installed and in your path. Of course, your mod is better (as the script uses bash already), but there's bound to be some other occurences of count elsewhere, so I wouldn't commit that change.

That's a good idea! I'll do that.

Strangely, all files have CRLF line endings in my case, but none show up as changed in git. It must be something strange with how I cloned the repo - I'll look into it.

Ah ok, I would have never guessed that count is a STIR script! Where is the source code for it? The usage in the shell script gives me a ": invalid option" error.

Strangely, all files have CRLF line endings in my case, but none show up as changed in git. It must be something strange with how I cloned the repo - I'll look into it.

git config --global core.autocrlf input and all that, I guess

Ah ok, I would have never guessed that count is a STIR script! Where is the source code for it? The usage in the shell script gives me a ": invalid option" error.

https://github.com/UCL/STIR/blob/master/scripts/count

My guess this is also a CRLF thing. I think we need to add it in .gitattributes (*.sh files are already taken care of)

Also, if any of this was automatic (probably not), please add the relevant commit to .git-blame-ignore-revs

Yes, unfortunately all manual!

sorry, could you update release-6.3.htm as well? Or even add minimal info to the STIR developer's guide (if appropriate). Commit again with `[ci skip]'. Thanks!