Simplify maintenance of DNA generation #37228

New Issue

Lawrence D'Oliveiro · 2013-10-28T02:05:34+01:00

2013-10-28 02:05:34 +01:00

%%%Currently the names of the DNA_*.h include files are listed in 3 separate places: in source/blender/CMakeLists.txt, and twice in source/blender/makesdna/intern/makesdna.c, and any additions/deletions require updates in all 3 places. This patch puts in a single master list (in the DNA_LIST variable in source/blender/CMakeLists.txt), which is the only one that needs to be maintained by hand; all the other lists are automatically generated from that at build time.
%%%

%%%Currently the names of the DNA_*.h include files are listed in 3 separate places: in source/blender/CMakeLists.txt, and twice in source/blender/makesdna/intern/makesdna.c, and any additions/deletions require updates in all 3 places. This patch puts in a single master list (in the DNA_LIST variable in source/blender/CMakeLists.txt), which is the only one that needs to be maintained by hand; all the other lists are automatically generated from that at build time. %%%

Lawrence D'Oliveiro commented

2013-10-28 02:05:34 +01:00

Changed status to: 'Open'

Lawrence D'Oliveiro commented

2013-10-28 02:09:50 +01:00

%%%Of course, one could go further than this, and use FILE(GLOB ...) to automatically generate the list at build time. the CMake docs recommend against this, because it means you have to remember to rerun CMake if files are added/removed. Would this be a big hardship?%%%

Campbell Barton commented

2013-10-29 00:57:59 +01:00

%%%Re globbing - yep, it is a big hardship.
see: http://stackoverflow.com/questions/1027247/best-way-to-specify-sourcefiles-in-cmake/18538444#18538444

However Im not sure about the patch, its de-duplicates at the expense of adding some minor complexity in makesdna,
also - scons support would have to be added.

I would prefer to leave this as-is, mainly because of having to add a generated file.%%%

%%%Re globbing - yep, it is a big hardship. see: http://stackoverflow.com/questions/1027247/best-way-to-specify-sourcefiles-in-cmake/18538444#18538444 However Im not sure about the patch, its de-duplicates at the expense of adding some minor complexity in makesdna, also - scons support would have to be added. I would prefer to leave this as-is, mainly because of having to add a generated file.%%%

Lawrence D'Oliveiro commented

2013-10-29 01:04:46 +01:00

%%%If you insist, I’ll look at adding an SCons patch :). (Why do we need two build systems, anyway?)

As for adding another generated file—isn’t the whole point of the computer to save us manual work? Any programmer worth their salt should consider laziness a virtue. :)%%%

%%%If you insist, I’ll look at adding an SCons patch :). (Why do we need two build systems, anyway?) As for adding another generated file—isn’t the whole point of the computer to save us manual work? Any programmer worth their salt should consider laziness a virtue. :)%%%

Lawrence D'Oliveiro commented

2014-01-20 04:55:29 +01:00

Here makesdna-simplify-20140120.patch is a resubmission of the patch, updated to work with both SCons and CMake.

Here [makesdna-simplify-20140120.patch](https://archive.blender.org/developer/F70879/makesdna-simplify-20140120.patch) is a resubmission of the patch, updated to work with both SCons and CMake.

Campbell Barton commented

2014-01-20 06:59:45 +01:00

Checked the patch again, Its quite good but I still can't really justify committing it (maybe other devs have different opinions...)

Some observations...

It stores part of a C file in the build system, since its done for CMake and SCons its done twice...
I checked on storing this in a template file, so both scons and cmake could reuse, but this adds a further complication that a change to the template needs to trigger cmake to re-run, to generate the output from the template. (This is possible but not so easy. I gave up on the idea at this point).

Generating dna_list.c each time causes the build system to re-run, needlessly rebuilding, ideally it would generate the file, then copy-if-different (comparing to the previously generated file), I tested this out for cmake and got it working, but was apart of the patch to use an external template (which I gave up on).

Overall Feedback:

To make this patch as good as I think it could be, it would need to check for changes before re-creating the C source file.
Minor note for SCons - it could make use of triple-quoted text for muli-line string literal.

Even with these improvements, personally wouldn't want to accept this patch, just think the gain in complexity isnt worth the improvement of de-duplication.

However interested if other devs would like to put their 2c in, maybe it can be made to work better in some way I overlooked.

Checked the patch again, Its quite good but I still can't really justify committing it (maybe other devs have different opinions...) Some observations... It stores part of a C file in the build system, since its done for CMake and SCons its done twice... I checked on storing this in a template file, so both scons and cmake could reuse, but this adds a further complication that a change to the template needs to trigger cmake to re-run, to generate the output from the template. (This is possible but not so easy. I gave up on the idea at this point). Generating `dna_list.c` each time causes the build system to re-run, needlessly rebuilding, ideally it would generate the file, then copy-if-different (comparing to the previously generated file), I tested this out for cmake and got it working, but was apart of the patch to use an external template (which I gave up on). Overall Feedback: To make this patch as good as I think it could be, it would need to check for changes before re-creating the C source file. Minor note for SCons - it could make use of triple-quoted text for muli-line string literal. Even with these improvements, personally wouldn't want to accept this patch, just think the gain in complexity isnt worth the improvement of de-duplication. However interested if other devs would like to put their 2c in, maybe it can be made to work better in some way I overlooked.

Lawrence D'Oliveiro commented

2014-01-20 11:04:31 +01:00

Thank you for the thoughtful feedback.

I noticed a similar opportunity for refactoring makesrna, but I guess I’ll hold off on that for now. :)

Thank you for the thoughtful feedback. I noticed a similar opportunity for refactoring makesrna, but I guess I’ll hold off on that for now. :)

Lawrence D'Oliveiro commented

2014-01-30 23:15:09 +01:00

I’m not sure I understand your comment about ‘caus[ing] the build system to re-run, needlessly rebuilding”. The generation of ##dna_list.c## is only done once during the build, and it is only listed as a dependency of the ##makesdna## tool, so why should there be impacts on anything else?

The patch as currently submitted already knocks about 80 lines off the Blender source, besides removing one manual maintenance step which, while it may not need to be done frequently, is only more likely to be a source of errors because of that.

Yes, you need to rerun CMake after adding/removing files; don’t you have to, anyway?

I’m not sure I understand your comment about ‘caus[ing] the build system to re-run, needlessly rebuilding”. The generation of ##dna_list.c## is only done once during the build, and it is only listed as a dependency of the ##makesdna## tool, so why should there be impacts on anything else? The patch as currently submitted already knocks about 80 lines off the Blender source, besides removing one manual maintenance step which, while it may not need to be done frequently, is only more likely to be a source of errors because of that. Yes, you need to rerun CMake after adding/removing files; don’t you have to, anyway?

Lawrence D'Oliveiro commented

2014-01-30 23:34:46 +01:00

Resubmission makesdna-simplify-20140131.patch of patch, because I forgot to include the corresponding poisoning directive for SCons builds.

Resubmission [makesdna-simplify-20140131.patch](https://archive.blender.org/developer/F75679/makesdna-simplify-20140131.patch) of patch, because I forgot to include the corresponding poisoning directive for SCons builds.

Campbell Barton commented

2014-01-30 23:49:32 +01:00

Checked the patch, theres is still an issue with dna_list.c

Simple to redo.

build blender...
build again (it should be very fast now)
now run cmake . un the build dir *****
now build...

*****: Notice how running cmake builds dna_list.c and then dna.c, then rna_****_gen.c
Even though dna_list.c had no changes besides being re-created.

I think the fix for this is to only overwrite dna_list.c when the newly generated file differs from the old one.

Checked the patch, theres is still an issue with `dna_list.c` Simple to redo. - build blender... - build again (it should be very fast now) - now run `cmake .` un the build dir ***** - now build... *****: Notice how running cmake builds `dna_list.c` and then `dna.c`, then `rna_****_gen.c` Even though `dna_list.c` had no changes besides being re-created. I think the fix for this is to only overwrite `dna_list.c` when the newly generated file differs from the old one.

Lawrence D'Oliveiro commented

2014-05-28 02:48:49 +02:00

OK. Enclosed is an updated patch, that now also compares against the contents of any existing dna_list.c before deciding if it needs to be replaced.makesdna-simplify-20140528.patch

OK. Enclosed is an updated patch, that now also compares against the contents of any existing dna_list.c before deciding if it needs to be replaced.[makesdna-simplify-20140528.patch](https://archive.blender.org/developer/F91651/makesdna-simplify-20140528.patch)

Lawrence D'Oliveiro commented

2014-05-28 03:10:46 +02:00

Whoops, I got the comparison the wrong way round in the CMake patch. Herewith a replacement patch. makesdna-simplify-20140528-1.patch

Whoops, I got the comparison the wrong way round in the CMake patch. Herewith a replacement patch. [makesdna-simplify-20140528-1.patch](https://archive.blender.org/developer/F91652/makesdna-simplify-20140528-1.patch)

Lawrence D'Oliveiro commented

2014-05-28 03:48:06 +02:00

Another update: I think it is safest to quote those string substitutions in the comparison.

makesdna-simplify-20140528-2.patch

Another update: I think it is safest to quote those string substitutions in the comparison. [makesdna-simplify-20140528-2.patch](https://archive.blender.org/developer/F91659/makesdna-simplify-20140528-2.patch)

Sergey Sharybin commented

2015-02-26 12:03:10 +01:00

Added subscriber: @Sergey

Sergey Sharybin commented

2015-02-26 12:03:10 +01:00

I'm not really sure what this patch is aimed to solve/make easier. it just moves complexity from one thing to another, overall making it less clear what's happening in there IMO.

And in the bigger picture of adding new DNA it doesn't make much sense, because RNA still would need to have some manual includes of the new files.

I would say just having a clear page in wiki would be much more useful.

I'm not really sure what this patch is aimed to solve/make easier. it just moves complexity from one thing to another, overall making it less clear what's happening in there IMO. And in the bigger picture of adding new DNA it doesn't make much sense, because RNA still would need to have some manual includes of the new files. I would say just having a clear page in wiki would be much more useful.

Campbell Barton commented

2015-02-26 12:03:35 +01:00

Checked the patch, the way CMake and SCons have to in-line parts of the header is rather awkward, it could be some text extracted from a file in source control... however overall am not so positive about this patch.
de-duplicating some list is fine, but the complexity it adds just seems not to warrant the minor advantage we get.

Checked the patch, the way CMake and SCons have to in-line parts of the header is rather awkward, it could be some text extracted from a file in source control... however overall am not so positive about this patch. de-duplicating some list is fine, but the complexity it adds just seems not to warrant the minor advantage we get.

Lawrence D'Oliveiro commented

2016-05-22 04:10:26 +02:00

Here is an updated patch that should apply cleanly against the current source tree.

Bye-bye SCons...

makesdna-simplify-20160522.patch

Here is an updated patch that should apply cleanly against the current source tree. Bye-bye SCons... [makesdna-simplify-20160522.patch](https://archive.blender.org/developer/F314180/makesdna-simplify-20160522.patch)

Lawrence D'Oliveiro commented

2016-07-17 02:59:02 +02:00

Here is an updated patch that should apply cleanly against the current source tree.

makesdna-simplify-2016-07-17.patch

Here is an updated patch that should apply cleanly against the current source tree. [makesdna-simplify-2016-07-17.patch](https://archive.blender.org/developer/F324080/makesdna-simplify-2016-07-17.patch)

Lawrence D'Oliveiro commented

2016-08-07 00:10:52 +02:00

Here is an updated patch that should apply cleanly against the current source tree.

makesdna-simplify-2016-08-07.patch

Here is an updated patch that should apply cleanly against the current source tree. [makesdna-simplify-2016-08-07.patch](https://archive.blender.org/developer/F333606/makesdna-simplify-2016-08-07.patch)

Sergey Sharybin commented

2016-08-08 09:58:10 +02:00

Changed status from 'Open' to: 'Archived'

Sergey Sharybin closed this issue

2016-08-08 09:58:10 +02:00

Sergey Sharybin self-assigned this 2016-08-08 09:58:10 +02:00

Sergey Sharybin commented

2016-08-08 09:58:10 +02:00

This patch replaces one issue with another:

Using something what is more a workaround to have properly generated header file, which isn't really nice.
It ignores the fact that not all source files requires SDNA generated for.
It restricts order of files from which SDNA is generated (we might want some specific order for that, not just alphabetical one).

All in all i don't think advantage worth all this side effects of the patch. And i never heard this is to be an issue anyway.

So thanks for the efforts, but don't think it a;; worth it.

This patch replaces one issue with another: - Using something what is more a workaround to have properly generated header file, which isn't really nice. - It ignores the fact that not all source files requires SDNA generated for. - It restricts order of files from which SDNA is generated (we might want some specific order for that, not just alphabetical one). All in all i don't think advantage worth all this side effects of the patch. And i never heard this is to be an issue anyway. So thanks for the efforts, but don't think it a;; worth it.

Lawrence D'Oliveiro commented

2016-09-12 21:46:42 +02:00

Nothing of what you say makes sense. Are you sure you are commenting on the correct task?

“It ignores the fact that not all source files requires SDNA generated for” -- it only generates SDNA for the listed files. These are the files that were listed in makesdna.c, so you clearly wanted SDNA generated for them, otherwise why were they there?

“It restricts order of files from which SDNA is generated (we might want some specific order for that, not just alphabetical one).” -- WHAT alphabetical order? I just copied the order they were originally in. You want a different order? Put them in that order!

“Using something what is more a workaround to have properly generated header file, which isn't really nice”-- that is not even a coherent sentence.

I feel I am wasting my time with you.

Nothing of what you say makes sense. Are you sure you are commenting on the correct task? “It ignores the fact that not all source files requires SDNA generated for” -- it only generates SDNA for the listed files. These are the files that were listed in `makesdna.c`, so you clearly wanted SDNA generated for them, otherwise why were they there? “It restricts order of files from which SDNA is generated (we might want some specific order for that, not just alphabetical one).” -- **WHAT** alphabetical order? I just copied the order they were originally in. You want a different order? Put them in that order! “Using something what is more a workaround to have properly generated header file, which isn't really nice”-- that is not even a coherent sentence. I feel I am wasting my time with you.

Sergey Sharybin commented

2016-09-13 11:55:33 +02:00

Ok, in latest patch the order is more in the developer control. But it's still kind of stressing CMake with something it wasn't designed for.

Thing is: this patch tries to solve something which was never an issue for developers who works in this area.

Ok, in latest patch the order is more in the developer control. But it's still kind of stressing CMake with something it wasn't designed for. Thing is: this patch tries to solve something which was never an issue for developers who works in this area.

Lawrence D'Oliveiro commented

2018-02-07 05:52:07 +01:00

Here is an updated patch that should apply cleanly against the current source tree.

makesdna-simplify-2018-02-07.patch

“Stressing” CMake? CMake laughs at you.

Here is an updated patch that should apply cleanly against the current source tree. [makesdna-simplify-2018-02-07.patch](https://archive.blender.org/developer/F2163190/makesdna-simplify-2018-02-07.patch) “Stressing” CMake? CMake laughs at you.

Lawrence D'Oliveiro commented

2018-06-24 04:21:27 +02:00

Here is an updated patch that should apply cleanly against the current source tree.

makesdna-simplify-2018-06-24.patch

CMake eats up Blender builds for breakfast and asks “What’s for lunch?”.

Here is an updated patch that should apply cleanly against the current source tree. [makesdna-simplify-2018-06-24.patch](https://archive.blender.org/developer/F3784467/makesdna-simplify-2018-06-24.patch) CMake eats up Blender builds for breakfast and asks “What’s for lunch?”.

Lawrence D'Oliveiro commented

2018-12-22 01:56:12 +01:00

Here is an updated patch that should apply cleanly against the current source tree.

ldo-makesdna-simplify-2018-12-22.patch

That’s three times this year my patch could have saved you some work!

Here is an updated patch that should apply cleanly against the current source tree. [ldo-makesdna-simplify-2018-12-22.patch](https://archive.blender.org/developer/F6030306/ldo-makesdna-simplify-2018-12-22.patch) That’s three times this year my patch could have saved you some work!

Sign in to join this conversation.

No Label

Download

What's New

Blender Studio

Manual

Developers Blog

Documentation

Benchmark

Blender Conference

Development Fund

One-time Donations

Simplify maintenance of DNA generation #37228