[cmake:]fix the dependence to avoid generate files every time, even though no changed

[xiaoyuLi-China0372]

No description provided.

[mikkelfj]

Thanks for this PR. There is an old PR that wasn't merged addressing the same issue. I asked for input, as per my mention above.

Also, does this work with both Ninja and Make backends? The CI build works with both, but that does not test rebuilds. I had a lot of problems getting rebuilds to work reliably with Ninja.

[xiaoyuLi-China0372]

Thanks for this PR. There is an old PR that wasn't merged addressing the same issue. I asked for input, as per my mention above.

Also, does this work with both Ninja and Make backends? The CI build works with both, but that does not test rebuilds. I had a lot of problems getting rebuilds to work reliably with Ninja.

Yes, It works with both of them, include rebuild

[mikkelfj]

Yes, It works with both of them, include rebuild

Thanks, that is good to hear. Just to be sure: have you tested that the rebuild works after modifying a single file, for example in a test .c file, or a .fbs file the test depends on, like monster_test.fbs? Also, .h files in two levels of inclusion can sometimes be a problem, and so can included .fbs files.

I'll wait to see if some of the other contributors has something to comment on.

[xiaoyuLi-China0372]

Yes, It works with both of them, include rebuild

Thanks, that is good to hear. Just to be sure: have you tested that the rebuild works after modifying a single file, for example in a test .c file, or a .fbs file the test depends on, like monster_test.fbs? Also, .h files in two levels of inclusion can sometimes be a problem, and so can included .fbs files.

I'll wait to see if some of the other contributors has something to comment on.

I refixed it, updated the commit. you can test it again

[mikkelfj]

I did not test all cases, but it does seem to work for incremental builds now, thanks.
Still waiting a while to see if others have input to this.

[bjosv]

Unfortunately this will break builds when using an older CMake version,
like the minimum required version 2.8.12.2:

...
CMake Error at samples/reflection/CMakeLists.txt:19 (list):
  list does not recognize sub-command TRANSFORM
  
CMake Error at samples/reflection/CMakeLists.txt:24 (add_custom_command):
  add_custom_command Wrong syntax.  A TARGET or OUTPUT must be specified.
...

If I checked correctly the TRANSFORM sub-command was added in 3.12 and the other issue might be a trailing error.

Maybe we should move/copy the CMake test-step from weekly to CI? ..and possibly only run it if a CMakeLists.txt has been changed. An issue like this would have been shown directly then.

[mikkelfj]

Maybe we should move/copy the CMake test-step from weekly to CI? ..and possibly only run it if a CMakeLists.txt has been changed. An issue like this would have been shown directly then.

Having it in CI too definitely wouldn't hurt (though we don't change CMake that often). The weekly builds are much faster than a feared, except possibly macOS, so we could be more aggressive with CI testing.

The discussion in the following PR may also be of interest. Note that a bit down I complain about how I had problems getting incremental builds working with Ninja.

#169

[mikkelfj]

If I checked correctly the TRANSFORM sub-command was added in 3.12 and the other issue might be a trailing error.

I'm not opposed to upgrading CMake at this point in time. The question is only to which version. It needs to be installable on most systems and supported trivially on fairly recent Ubuntu LTS as a minimum, as a guiding principle.

[mikkelfj]

Hi, sorry for the delay.
I bumped to CMake 3.18.4 experimentally on the new branch "cmake: (will be deleted later) and then merge it with this PR in the new branch "pr258-merge" (will be deleted later). You can see these branches in the repo for now.

The github workflow build passes but the appveyor build fails because the CMake version is too new. This also happens on the cmake branch. The good news is that the pr works with github actions.

The reason I chose 3.18.4 is that this is latest CMake supported by Debian stable.

However, our CI build is not fully ported to github workflows even if it does have some windows builds, and I cannot be bothered to try to update the Appveyor build to a version that might support a more recent version of CMake.

So I will probably let this PR sit until we get better windows support on github workflows.

@bjosv do you have any opinion on this?

[bjosv]

One option could be to use 3.12 as the minimum required CMake version since that would be the oldest version a user can have to successfully generate build files, independently of OS/distribution.
That would also "solve" the appveyor problem..

One thing to know is that the version set in cmake_minimum_required also might change behaviors of a newer CMake.
It will use this to determine which policies/behaviors that were enabled by default at this version, and only enable those policies. A backward compability thing.
I'm not that familiar with it, just wanted to mention that there is a long list of them..

[bjosv]

I could look into adding some more Windows tests, but Github Actions only provides windows-2022 with Visual Studio Enterprise 2022, and windows-2019 with Visual Studio Enterprise 2019.
Appveyor seem to have a better variety of older images like 2015..

[mikkelfj]

I could look into adding some more Windows tests, but Github Actions only provides windows-2022 with Visual Studio Enterprise 2022, and windows-2019 with Visual Studio Enterprise 2019.
Appveyor seem to have a better variety of older images like 2015..

If your are willing to do so, I can dig into installing older MSVC back to 2013 I think. I may have to PM you on specifics as it is under some unreleased software. I don't mind dropping MSVC 2010, it is not even a C compiler, really.

[mikkelfj]

Heads up on this PR after finally getting some time to work on this.
My plan is to make a release other changes first. Then add build changes from this PR and #262 that adds meson build support too. I am not too concerned with Appveyor at this point (as stated above Appveyor at least used to fail on newer CMake version), I'd rather eventually update GH actions for older windows builds, but even if we miss that, those users can use older releases if they really need that.

As to CMake versions, Ubuntu 20.04 LTS uses CMake 3.16, Debian just made a new stable 12 "bookwork" with CMake 3.25, and CMake generally tends to complain about CMake < 3.5 not being supported in the future.
Thus it makes sense to bump to CMake 3.16, but as said, I'll make a release first.

[mikkelfj]

I am no CMake expert, but there may also be some good ideas in the PR #168 that I just closed, as I cannot have all these overlapping build changes.