Skip to content
New issue

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

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

Already on GitHub? # to your account

Build Metadata in src not include #600

Closed
5 tasks done
ptheywood opened this issue Jul 21, 2021 · 1 comment · Fixed by #662
Closed
5 tasks done

Build Metadata in src not include #600

ptheywood opened this issue Jul 21, 2021 · 1 comment · Fixed by #662

Comments

@ptheywood
Copy link
Member

ptheywood commented Jul 21, 2021

Currently, the version build metadata (i.e. commit hash) is stored in include/flamegpu/version.h, which is modified at every cmake configuration event.

This breaks out of tree builds, but is a current neccesary evil to minimise the use of the incorrect include directory for RTC compilation (potentially only for non-python RTC?)

Instead, it would be preferable if flamegpu::VERSION_BUILDMETADATA was declared in version.h and defined in a dynamic file in the build directory (i.e. build/flamegpu/version.cpp), so that the include directory is not modified by cmake.
This will prevent the use of it while checking the include directory version, instead the major, minor and prerelease version would need to be sufficient. This would be OK for users of stable releases, but may be insufficient during development.

One possible workaround for this would be to add a new CMake option, or CLI/environment varible option to ignore the jitify cache, to avoid incorrect cache hits, however it might still cause some issues during development.
Another solution would be to dynamically create version.h in the build directory, pulling in a copy of the headers and using that as the FLAMEGPU_INC_DIR?

It's unclear what is the best option.

  • Stop include/flamegpu/version.h from being generated by CMake
    • Encode the version information in the header manually, and extract into CMake. Inverting the relationship. Include the pre-release version number in the header. I.e. alpha.1 as a constexpr string literal.
    • Declare but do not define extern const char VERSION_BUILDMETADATA[];, implemented as static.
    • Adjust RTC include directory checking to not consider build metadata (subject to any alternatives found). Only major/minor/patch/prerelease should be sufficient?
      • We may want to add some form of development mode cmake version that changes RTC include directory behaviour? This could include embeding the include/ directory in binaries (1.5MB currently)? Continuing without for now.
  • Generate build/flamegpu/version.cpp via CMake at configure time, including the git commit hash as build meta data, embedding the build metadata in the static library.
@ptheywood
Copy link
Member Author

After a discussion it was agreed to proceed without the opt-in metadata include directory check, as the RTC API should be stable enough that this isn't a problem. If it does turn out to be an issue, we could potentially do one or more of:

  • Add an cmake configuraiton option for a developer mode, which will embed the version in the include directory
  • Copy the include directory into the build directory, which would be found before the parent include dir (if the appropriate env var is not set). Might lead to other issues, such as editing the wrong files, or running without reconfiguring and being thrown off.

@ptheywood ptheywood added this to the v2.0.0-alpha.1 milestone Sep 1, 2021
# for free to join this conversation on GitHub. Already have an account? # to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant