Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Generate version header #1920

Merged
merged 11 commits into from
Mar 9, 2020
Merged

Generate version header #1920

merged 11 commits into from
Mar 9, 2020

Conversation

ZedThree
Copy link
Member

This solves a problem with the CMake system: everything gets rebuilt on every commit because -DBOUT_VERSION* are passed on the command line to every file, and CMake knows that if the preprocessor macros for a file change then the file needs to be rebuilt.

This is solved by adding include/bout/version.hxx which is generated by the build system from include/bout/version.hxx.in. This header contains constexpr variables in the bout::version:: namespace, and replaces the preprocessor macros BOUT_VERSION, REVISION, BOUT_VERSION_STRING and BOUT_VERSION_DOUBLE.

It might be nice to mark those macros as deprecated somehow. Not found a nice portable way so far!

There's a little funkiness with the makefile build system: BOUT_REVISION (renamed from REVISION) is still passed to each file via the command line. I couldn't see a way round this, which is why version.hxx.in still has an #ifndef in it.

Also, there is a potential issue here: if in one cxx file you take the address of a constexpr variable defined in a header, you may/will get a different address than in another cxx file. C++17 solves this with inline variables.
Mostly we shouldn't be taking the address of constexpr variables, but unfortunately, this is how Datafile::add works. Basically, don't call Datafile::add on the same bout::version variable twice in different files, or it will throw. But you probably shouldn't be doing that anyway!

Lastly, there's a bunch of unrelated changes to configure because I think the gettext macro has been updated on my system. Happy to pull those changes out into a separate PR.

By putting the #define in a header rather than a compile-line
definition, and using that header only in the file that needs it,
CMake doesn't need to recompile every file on commit
Note that for the makefile build system, BOUT_REVISION is still a
preprocessor macro passed on the command line
@ZedThree ZedThree added this to the BOUT-5.0 milestone Feb 25, 2020
@johnomotani johnomotani added the breaking change Introduces a change that is not backward compatible with the previous major version label Feb 25, 2020
@johnomotani
Copy link
Contributor

I guess this is a breaking change. People probably not using BOUT_VERSION, etc. but if they are the names have changed. Needs adding to a 4to5 script?

@ZedThree
Copy link
Member Author

Yes, probably wise!

bout.hxx ends up included in a bunch of library files, so will still
trigger some compilation cascade on commit
Makefile-build system now also configures this file to include the
commit from configure-time, so that bout::version::revision will
always have a meaningful value even if -DBOUT_REVISION is not passed
on command line
@ZedThree
Copy link
Member Author

Possible gotcha: if you build with ./configure and then with CMake, it will pick up the makefile system version of include/bout/revision.hxx, not the CMake generated one.
make distclean now cleans up the generated headers.

I'm now happy that this does what we want for CMake: on commit revision.hxx is regenerated, only bout++.cxx gets recompiled, and all the executables only get relinked and not rebuilt.

bin/bout-v5-macro-upgrader.py does a "best effort" attempt to convert macros to variables, add the necessary headers, and remove unneeded #ifdefs. It doesn't attempt to expand other macros or fix #ifndefs as this got tricky very quickly: a simple replacement might not be correct.

johnomotani
johnomotani previously approved these changes Feb 28, 2020
Copy link
Contributor

@johnomotani johnomotani left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice! This works for me. Will save lots of time compiling when I have to commit everything to debug Travis runs...

Would be nice to add a section to the manual listing the breaking changes, and the scripts available to apply the necessary updates.

bin/bout-v5-macro-upgrader.py Show resolved Hide resolved
@johnomotani
Copy link
Contributor

It doesn't attempt to expand other macros or fix #ifndefs as this got tricky very quickly: a simple replacement might not be correct.

Would it be worth adding some warnings to flag up any uses that weren't fixed? The compiler should give errors there anyway, but I think it would be friendlier if this script gave a warning.

@ZedThree
Copy link
Member Author

Good point. We're now not defining those macros, so #ifndef sections will suddenly get compiled.
Maybe remove the #ifndef lines and also flag a warning?

johnomotani
johnomotani previously approved these changes Feb 28, 2020
@ZedThree
Copy link
Member Author

ZedThree commented Mar 2, 2020

This now takes care of situations like the contrived:

-#ifndef BOUT_VERSION_STRING
-#ifdef REVISION
-  output_progress.write(_("BOUT++ version {:s}\n"), BUILDFLAG(REVISION));
-#else
-  output_progress.write(_("BOUT++ version {:s}\n"), "Unknown");
-#endif
-#else
-  output_progress.write(_("BOUT++ version {:s}\n"), BOUT_VERSION_STRING);
-#endif
-#ifdef REVISION
-  output_progress.write(_("Revision {:s}\n"), BUILDFLAG(REVISION));
-#endif
-#ifndef REVISION
-  output_progress.write(_("Revision {:s}\n"), "Unknown");
-#endif
+  output_progress.write(_("BOUT++ version {:s}\n"), bout::version::full);
+  output_progress.write(_("Revision {:s}\n"), BUILDFLAG(bout::version::revision));

I consider all these upgrader scripts as "best effort only" and somewhat dangerous. The output must be checked by the users, so there's no specific warning for this one. When I get them all into a single framework type thing, it'll have a big flashing warning.

@ZedThree ZedThree merged commit 1c639bd into next Mar 9, 2020
@ZedThree ZedThree deleted the generated-revision-header branch March 9, 2020 10:59
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
breaking change Introduces a change that is not backward compatible with the previous major version
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants