cornelius / kode

XML meta programming
http://www.lst.de/~cs/kode
20 stars 10 forks source link

Do not overwrite output files if not necessary #32

Closed martonmiklos closed 4 years ago

martonmiklos commented 4 years ago

Do not overwrite output files in the case their content did not get changed.

This is useful because in this case the project including the generated files will not need to recompile the unchanged files.

Fixes #30

martonmiklos commented 4 years ago

@dfaure-kdab may I ask you for a review?

It might be useful to merge this functionality at some point to the KDSoap as well.

martonmiklos commented 4 years ago

not used anymore

@dfaure-kdab Ah I force pushed in the meantime, so github lost the reference to this comment. Could you please point out to the location of this comment?

Thanks, and I apologize for force pushing during review requested state.

martonmiklos commented 4 years ago

toUtf8()

Fixed.

martonmiklos commented 4 years ago

Simpler: if ( file->exists() )

Makes sense, fixed.

dfaure-kdab commented 4 years ago

The addition of "class QFile;" in a public header was the unnecessary bit now that the method is in the private class.

martonmiklos commented 4 years ago

The addition of "class QFile;" in a public header was the unnecessary bit now that the method is in the private class.

Ah thanks, fixed!

martonmiklos commented 4 years ago

Many thanks for your time and patience David!

martonmiklos commented 4 years ago

@dfaure-kdab Btw. do you think that it would worth merging this to the KDSoap? In the case if someone tweaking a WSDL and Qt code at the same time it might be helpful to reduce the unnecessary compilations.

dfaure-kdab commented 4 years ago

Yes this would probably be a good idea.

Although if you tweak the WSDL, the generated output is going to be different so this won't help :-) Probably much more useful for people working on kdwsdl2cpp itself. There's a dependency on the executable, so if I change even a comment in the kdwsdl2cpp code, everything gets regenerated and rebuilt. +1 for a KDSoap merge ;)

martonmiklos commented 4 years ago

Probably much more useful for people working on kdwsdl2cpp itself.

Makes sense. I did not remember why exactly implemented this feature, but yeah kxml_compiler polishing was the point.

I will came up with a PR in KDSoap soon™.

dfaure-kdab commented 4 years ago

This feature creates the following issue: because the generated files don't get "touched" (modification time update), every single run of "make" will re-attempt to regenerate the file again. Every time it will realize there's no change, but overall this seems slower than before this feature. At least it's very verbose... I see "Generating wsdl_foo.h" / "Generating wsdl_foo.cpp" every time. But of course if we actually changed the mtime of the generated file, it would get rebuilt, and it would be like reverting this commit altogether.

I'm somewhat undecided. It's the job of the buildsystem to not rebuild things for no purpose, and it's all based on modification times. Cheating the system (by not actually rebuilding what the buildsystem asks us to rebuild) seems counter-productive and actually creates more problems.

martonmiklos commented 4 years ago

What about adding a command line argument making it optional and disabled by default?

I apologize if I made trouble with this and of course if you agree with the additional argument approach I will fix it and submit a PR.

dfaure-kdab commented 4 years ago

That would be OK with me.

But I wonder what's the use for you? Surely not when working on KDSoap, since you'd have to modify tons of files to change the way kdwsdl2cpp is invoked. But if this isn't about working on kdwsdl2cpp itself, then I fail to see the point: in a separate project, the regeneration would only happen when the WSDL file is modified. Is this because you often modify comments/documentation in the WSDL file?

Thinking about it, all code generators do touch the file, if they are rerun, even if the output is the same. Try moc foo.cpp -o moc_foo.h, then do the same one minute later, the header file will be modified. This isn't normally a problem because this regeneration only happens if foo.cpp has changed, or if moc itself has changed.

So I realize now that I don't think this feature is a good idea. I'm used to "compare and don't touch if no change" for make install, there it makes a lot of sense, but not for generated files inside a project.

dfaure-kdab commented 3 years ago

@martonmiklos Based on the last comment here, I'm tempted to revert the commit. Can I have your input on that?

I don't see any other code generator having the feature (even optional) of not touching the generated file if the contents didn't change, because this means the buildsystem will try to rebuild it every single time.

Yes it means that when working on the code generator itself we are recompiling things that didn't change, but I prefer that over turning what should be a no-op (the second make in "make ; make") into a non-no-op (re-running the code generator over all the files another time, with much output).

dfaure-kdab commented 3 years ago

ok, or maybe just a qgetenv() in Printer::Private::printCodeIntoFile

martonmiklos commented 3 years ago

I don't see any other code generator having the feature (even optional) of not touching the generated file if the contents didn't change, because this means the buildsystem will try to rebuild it every single time.

Yes it means that when working on the code generator itself we are recompiling things that didn't change, but I prefer that over turning what should be a no-op (the second make in "make ; make") into a non-no-op (re-running the code generator over all the files another time, with much output).

Yeah, you are probably right that it was a bad idea to implement. I felt it important because I worked a lot on the generators at that time. I apologize for lacking feedback for a year on this one.

ok, or maybe just a qgetenv() in Printer::Private::printCodeIntoFile

I am fine with that approach as well, because I still feel that it could be helpful when tinkering Kode or KDSoap codes.

dfaure-kdab commented 3 years ago

OK, done in https://github.com/cornelius/libkode/pull/45