ctubbsii commented on code in PR #2241: URL: https://github.com/apache/zookeeper/pull/2241#discussion_r2085164605
########## zookeeper-client/zookeeper-client-c/src/main/c-filtered/include/zookeeper_version.h: ########## @@ -0,0 +1,31 @@ +/** Review Comment: > Currently, this pr require additional maven build for changes in cmake/automake files. Prior to my change in b52ad0a, the extra build was only necessary to ensure the version is correct. It would still without an extra Maven build, but might have an old version. The version doesn't change very often, so I originally did it that way, because I thought that would be enough. However, you mentioned that it might be confusing to have two files (the template and the result) both in the source tree. My change in b52ad0a fixed that, leaving only the template, and ignoring the generated file. Now, the extra Maven build is necessary to get the C build to work at all, but the benefit is that no files with outdated versions will get checked in to git. > I don't think it is worth for us to go this direction. I'm not clear which direction you are talking about. Are you referring to the change to ignore the generated file in b52ad0a? Or, are you talking about this PR overall? Because, I think this PR still has benefits overall: 1. It helps keep the version up-to-date on every build (not just releases), 2. It simplifies the release profile and reduces the number of commits added during the release profile, and 3. It removes the use of the antrun plugin and removes the dependency tree from the build for that plugin. 4. (I also included a bugfix for JMockIt in 2931f53) > Sorry for my rollback! I don't think the extra maven build is good for c client. If you are just talking about the change in b52ad0a, I can revert that part of this PR so the extra build isn't necessary (but keep the comment about the file being generated, to reduce confusion for developers making modifications). The consequence is that the version will be out-of-date only if the C client is built without doing the Maven build first. But, that would probably be okay, because that's rare. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: notifications-unsubscr...@zookeeper.apache.org For queries about this service, please contact Infrastructure at: us...@infra.apache.org
