Page MenuHome

CMake: create readme.html with configure_file
ClosedPublic

Authored by Ankit Meel (ankitm) on Jan 18 2022, 8:18 AM.

Details

Summary

Create readme.html using configure_file:

  • Now it's modified only if final output changes (handled by CMake).
  • If input file (from git) or blender version changes, it will be modified.

Since the output file stays unmodified for most developer runs,
install step installed it redundantly.

Also don't re-implement what CMake can do.


Will need to test the remove_recurse part, so will not land that piece of code now. I had posted this diff to use phab as git stash but here we are (:

Diff Detail

Event Timeline

Ankit Meel (ankitm) requested review of this revision.Jan 18 2022, 8:18 AM
Ankit Meel (ankitm) created this revision.
Ankit Meel (ankitm) retitled this revision from cmake: don't delete files at install; replace install code with configure_file to cmake: don't delete files at install; install readme.html with configure_file.Jan 18 2022, 8:22 AM
Ankit Meel (ankitm) updated this revision to Diff 47192.
release/text/readme.html
91

irc needs update too

This revision is now accepted and ready to land.Feb 14 2022, 11:11 PM

Patch description could use some work though, the commit message has to be better than this.

Campbell Barton (campbellbarton) requested changes to this revision.EditedFeb 15 2022, 1:29 AM

*Edit* I had a concern about this being moved out of the install target into the CMake configuration step.

It turns out this is reliable (updating the version number in BKE_blender_version.h is guaranteed to re-run CMake), so there are no problems with the readme.html not explicitly depending on BKE_blender_version.h.

If the user manually removes readme.html the install target will fail but developers shouldn't be manually removing files created by CMake in the build directory so I don't think it's a problem.

If for some reason this ends up causing problems we could always move the configure_file(...) inside an install(CODE ...) command.

This revision now requires changes to proceed.Feb 15 2022, 1:29 AM
This revision is now accepted and ready to land.Feb 15 2022, 1:40 AM
source/creator/CMakeLists.txt
1133

Worth noting that this runs on CMake configuration which always re-runs when the version changes.

Also wrap over two lines as it's >120 columns.

Ankit Meel (ankitm) retitled this revision from cmake: don't delete files at install; install readme.html with configure_file to CMake: create readme.html with configure_file.Feb 15 2022, 5:28 AM
Ankit Meel (ankitm) edited the summary of this revision. (Show Details)
Campbell Barton (campbellbarton) requested changes to this revision.Feb 15 2022, 6:07 AM

@Ankit Meel (ankitm) I missed the removal of "file(REMOVE_RECURSE ${TARGETDIR_VER})" as far as I can see this is still important to keep for the reason noted in the comment, surely configure_file(..) can be made to work without such a big change.

This revision now requires changes to proceed.Feb 15 2022, 6:07 AM
Ankit Meel (ankitm) marked an inline comment as done and an inline comment as not done.

@Ankit Meel (ankitm) I missed the removal of "file(REMOVE_RECURSE ${TARGETDIR_VER})" as far as I can see this is still important to keep for the reason noted in the comment, surely configure_file(..) can be made to work without such a big change.

Yes that's unrelated. As said it was a personal stash before I discarded local changes. Was not ready for review when reviewers were added.

Ankit Meel (ankitm) edited the summary of this revision. (Show Details)Feb 15 2022, 6:12 AM
This revision was not accepted when it landed; it landed in state Needs Review.Feb 15 2022, 6:13 AM
This revision was automatically updated to reflect the committed changes.