ExernalProject.cmake typo?

I was investigating why a patch step was not running using FetchContent.
Is it possible line 3900 has a typo?

foreach(step IN ITEMS download update parse)

Should that parse be a patch?

Yes, that looks like it should be patch. Feel free to submit an MR.

Cc: @craig.scott

Yes that’s a regression introduced in CMake 3.20.0-rc1. I’ll put up a fix myself shortly. Thanks for spotting it.

I need to research the documents on how patch application is handled too, since the patch step is getting run all the time. The .cmake script is always newer than the stamp and the stamp depends on the .cmake script.

The patch command depends on the update command. The update command always runs because it has to check if any update needs to be applied (things external to the build can change, like the upstream repo that the download step uses could have had more commits added, branches and tags moved, etc). Since the update step is always seen as out of date, the patch command will also always run. I think the only exception to this is if there is no update command (a URL download method would fall into this category). In that case, the update step is not always treated as out of date, so the patch command should only run if the download details change or after the first download.

If you are seeing the patch step running now where it didn’t with CMake 3.19, please report that as a regression in the issue tracker.

1 Like

Am currently building 3.19 to test just that. Will file an issue if need be.

I think you may be right though. I think I see why you are seeing the time stamps getting updated when they shouldn’t be. It will affect more than just the patch command too. :disappointed:

I’m still trying to come up with a simple failing test case.

Don’t spend too long on that, I’m already part way through creating a test case and already have the fix (it’s small).

I have it now. It was worth it just as a learning exercise.

I’ll have to wait to put up the fix for this timestamp issue, as it relies on the earlier fix for the patch command typo. Should find its way to being merged in a couple of days.