THRIFT-4532: Do not update previously generated output files if the contents have not changed

[mustafa-cosar]

I also implemented a test

[mustafa-cosar]

Some of the tests are failing on travis-ci because haskell package server is down. However my test passed in appveyor and the related job in travis-ci.

Edit: At least it was on my fork... Here it seems to be fine.

[jeking3]

Was it truly necessary to change the signature of the methods from ostream to ofstream? After all, an ofstream is an ostream. It should be possible to keep all the methods using ostream which accepts your custom ofstream class that checks to see if the content changed.

Extra nice to see a test for this, by the way, I like that a lot!

[bgedik]

@jeking3 From what I see ofstreams (more specific) were converted to ostreams (more general).

[mustafa-cosar]

@jeking3 It is as @bgedik says. The function signatures were using ofstream mostly. I converted them to ostream.

[jeking3]

I see, thanks, I misread the diff.

[jeking3]

Given this is a breaking change it should probably be noted somewhere in case anyone else has a custom compiler. Would this have any effect on a compiler plug-in being binary compatible?

[bgedik]

I was thinking that this won't break existing users of the Thrift compiler. Do you have a particular scenario in mind, where this can break things? It is behavior change, in the sense that the launch of the compiler does not regenerate all of the target files. I assume this can be mentioned in the release notes.

About the plug-ins: Do you have something like the Maven plug-in in mind? I guess the question is, would this break plug-ins that rely on the Thrift compiler binary. I am thinking that it won't, but it is something to be investigated. @mustafa-cosar Can you check if this behavior change would have any impact on the maven thrift plugin?

[mustafa-cosar]

@bgedik I don't think @jeking3 was talking about the maven plugin. From what I understand people can write their own generators and use it to generate their code.

I'm not too sure about how it works but I didn't make any changes to t_generator base class since it was already using ostream parameters.
Though I changed some function signatures to use ostream instead of ofstream in t_oop_generator and others.

[bgedik]

In that case, this will break binary compatibility. Because, in C++, changing the type of a function parameter in a shared library API will require a recompilation of the dependent applications. Here is a good list of do's and don'ts for binary compatibility: https://community.kde.org/Policies/Binary_Compatibility_Issues_With_C%2B%2B

It says 'changing any of the types of the arguments in the parameter list, including changing the const/volatile qualifiers of the existing parameters (instead, add a new method)' is among the things that break binary compatibility.

@jeking3 If this is really a concern, there is a simple solution to it. In t_oop_generator we can keep the old versions as additional overload, forwarding them to the new versions. You' are in a better position to judge. My thinking is that it is better to document this in the release notes only.

[mustafa-cosar]

Hi @jeking3 ! If everything's OK with you, can you merge this PR?
If you have any ideas for improvement for this issue, I'm happy to be of help!

[jeking3]

Hi, sorry I just switched jobs so my time here has been lower than normal the last couple weeks. I would recommend documenting this as a breaking change for anyone who has written their own compiler output module. Problem is that I don't see a good place to document breaking changes outside language-specific readme file. Documenting breaking changes has not been a strong suit for the project; while I'd love to improve it, perhaps for this it isn't necessary. I restarted the two travis failing build jobs for you.

[bgedik]

@mustafa-cosar Can you please update the readme file?

[jeking3]

Given this one passed all builds I'm going to merge it, I can update the readme separately.

[jeking3]

I updated the top level CHANGES file with a list of breaking changes since 0.11.0 that I could find through diffing README files.

[anorm]

This PR breaks compatibility with any build system that uses time stamps to check if a file needs to be regenerated (make, ninja, etc). If the .thrift file is touched, make correctly decides to re-run the thrift compiler. Thrift does not touch the generated files and next time make is run, the same thing happens again.

At the very least, this should have had a command line option to disable.

[jeking3]

If the contents of the output files have not changed, why would you need to recompile?

[anorm]

Any timestamp based build system (like make) will see that the generated files are older than the .thrift file and that they need to be rebuilt. This is in the "contract" between make and the different tools it invokes - the compiler, the linker, etc - if told to do something, they will do it.
It's not that files need to be recompiled for the resulting executable to be correct, but it's a requirement for timestamp based build systems that the timestamp of the generated files are updated.