Avoid rebuilding 100+ OpenImageIO files when rerunning CMake due to rewriting the fmt proxy headers (external fmt case)

[Willem871]

Description

OpenImageIO uses some proxy fmt headers, created at CMake time, that have a different content depending on using an external or the embedded fmt library.
In case of using an external fmt (i.e., OIIO_USING_FMT_LOCAL and INTERNALIZE_FMT are OFF), the current CMake code uses "file (WRITE)" to write the fmt proxy headers, which always overwrites existing files. Hence, when rerunning CMake the timestamp of those headers is always updated, triggering a rebuild of a whole bunch of OpenImageIO files.

To fix this I propose to use configure_file, which is also recommended by the CMake documentation of file(WRITE), see https://cmake.org/cmake/help/latest/command/file.html.
This will only overwrite the output file when the content of the input file has changed.

Tests

Not applicable, this is just a build improvement.

Checklist:

• I have read the contribution guidelines.
• I have updated the documentation, if applicable. Not applicable
• I have ensured that the change is tested somewhere in the testsuite
  (adding new test cases if necessary). Not applicable, build stuff.
• If I added or modified a C++ API call, I have also amended the
  corresponding Python bindings (and if altering ImageBufAlgo functions, also
  exposed the new functionality as oiiotool options). Not applicable, since CMake
• My code follows the prevailing code style of this project. If I haven't
  already run clang-format before submitting, I definitely will look at the CI
  test that runs clang-format and fix anything that it highlights as being
  nonconforming.

[linux-foundation-easycla]

The committers listed above are authorized under a signed CLA.

• ✅ login: lgritz / name: Larry Gritz (e3e3dc3)
• ✅ login: Willem871 (f74fa70, 40fc8ce, 30532a2, efe0f2c, 9916390, 018d7be, 1c703d5)

[Willem871]

I've still updated some whitespace inconsistency in the CMake file.

Regarding the CLA, is it sufficient to get written approval from my director for a change this small?
This will only happen in the beginning of January though.

[lgritz]

If I understand correctly, you are using configure_file to hijack its ability to run only if the file is not already created, right? But doing so at the expense of adding three .in files and needing to transform them even though there is no actual configuration or variable substitution.

I wonder, then, if a simpler alternate approach might be to stick with file(WRITE, ...) but to surround it in an if that only does the write if the file doesn't already exist? Would that also solve the problem of not writing it unnecessarily (and therefore triggering recompiles), but without needing to add and transform the .in files?

[lgritz]

Regarding the CLA, is it sufficient to get written approval from my director for a change this small? This will only happen in the beginning of January though.

Unfortunately (depending on how you look at it), it's a one-size-fits-all automated system where the bot rejects PRs that don't have the EasyCLA e-signed.

I wish there was a "de minimis changes bypass the CLA" policy, but there isn't, and I understand why in practice that would just lead to more grey areas and uncertainty.

[lgritz]

Hi, @Willem871, this is a good catch and not something I was thinking about with those proxy files.

[using configure_file] will only overwrite the output file when the content of the input file has changed.

That would certainly work, but in this case the content is static and won't "change" -- it's just a matter of whether we write the file at all or not. So as I said in the inline comment, even configure_file is heavier than we need here, because I think all we need to do is write conditionally based on whether the file already exists, and I believe that will accomplish the same thing without needing to add the extra .in files.

[Willem871]

"Hijacking the configure_file function" sounds a bit too hard. ;-) The configure_file(COPYONLY) approach is really a valid use case (which I actually prefer in general), but you are right that for these static proxy headers it is also fine to use "if(NOT EXISTS)".

I'm not sure if I will get the CLA set up properly in my company, but I can of course ask this in the beginning of next year. But feel free to submit this fix yourself :-).

[lgritz]

configure_file(COPYONLY) is a valid use case, yes, but it requires the extra .in files. I think that the if approach is just simpler.

There's no rush on this, just let us know when you're able to get the CLA squared away. Hope you had a good holiday season.

[lgritz]

Code LGTM and passes CI, but we still need the DCO signoff and CLA before we can merge it.

[Willem871]

Code LGTM and passes CI, but we still need the DCO signoff and CLA before we can merge it.

The CLA should be ok now!

[lgritz]

@Willem871, can you please mark this as ready to review, it's currently marked as a draft. Thanks.

[Willem871]

Sorry for the delay, I'm not really familiar with all the processes in Github. Hence, I didn't notice there was also a "request re-review" button... ;-)

[lgritz]

Sorry, you shouldn't have to ask for re-review, I try to look at pending PRs periodically and make sure nothing is waiting for me, and I think I dropped the ball on this one.

The CLA is fine now, but I need you to add the DCO before we can merge. You can do that with

git commit --amend -s

and then push --force to update the PR.

[Willem871]

@lgritz, the DCO is added to the last commit. Is this sufficient? I tried to do this for all the commits but I messed something up with the rebase command. The last commit is also the only one containing a relevant change, the other commits have actually reverted each other.

[lgritz]

@lgritz, the DCO is added to the last commit. Is this sufficient? I tried to do this for all the commits but I messed something up with the rebase command. The last commit is also the only one containing a relevant change, the other commits have actually reverted each other.

The DCO enforcement bot does indeed expect it to be on every commit.

The easy way to do this is git rebase -i HEAD~6 (I think you had 6 commits), that will bring up your $EDITOR. Change the "pick" to "r" for each one and exit the editor, then it will let you edit each commit message in turn, for which you should append Signed-off-by: Your Name <your@email> at the end of the commit message.

When you're all done, each of your commits will be edited, so just push origin <your_branch_name> --force and that will replace the current PR.

If you just can't get this to work, I think I may have the ability to override the DCO bot and ensure that the one sign-off you have is on the squashed commit. I think it's clear from this conversation is that your intent is to pledge the provisions of the DCO to the entire PR, but logistically you had trouble affixing it to every commit, so I don't think I'm committing any sin by fixing it up for you.

[lgritz]

@Willem871 I'm happy to fix it up for you if the system lets me. I wanted to spell out the right way to do it from your end, but my intent is not to make you struggle with this mechanism. You have made it clear that you want to apply the DCO to the whole thing, and saying it here is good enough for me.

[Willem871]

@lgritz, I tried but unintentionally added a change to another file and I can't get it right immediately. So, I think it is indeed better that you properly fix it.
Sorry and thank you for your patience!

[lgritz]

Sorry, one more issue. For some reason, there's a change to .mailmap bundled into this? Can you please get rid of those changes so that when you look at the change set ("Files changed" tab at the top of this page) it ONLY shows the changes you intend to add with this PR?

I think a lot of the trouble has been due to your directly submitting the PR from your own "master" branch, and then repeatedly updating or remerging or something.

A more stable way to do it (for next time, I think it's too late now) is to make a "topic branch" (i.e., git checkout -b my-descriptive-name), make the changes there, and then submit that branch as the basis for the PR. That would allow you to update the branch, separately from re-syncing master to get our latest additions, totally independently and without them interfering with each other in any way.