kezhuw commented on code in PR #2241: URL: https://github.com/apache/zookeeper/pull/2241#discussion_r2086809489
########## zookeeper-client/zookeeper-client-c/src/main/c-filtered/include/zookeeper_version.h: ########## @@ -0,0 +1,31 @@ +/** Review Comment: > I'm not clear which direction you are talking about. Are you referring to the change to ignore the generated file in https://github.com/apache/zookeeper/commit/b52ad0a152c2b7c4086b82ac4bdbb62f345fb7a0? > > Or, are you talking about this PR overall? Sorry for the confusion from me! >The logic at https://github.com/apache/zookeeper/blob/master/zookeeper-client/zookeeper-client-c/configure.ac#L69-L74 suggests that there is a desire to support running these commands without running Maven first. > 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. I am talking this. It is not rare when someone is going to tweak it. S/he will have to do maven build after every tweaks to cmake/automake files which I think is not good. > reduces the number of commits added during the release profile I tasted the release process according to https://cwiki.apache.org/confluence/display/ZOOKEEPER/HowToRelease+using+maven+release+plugin. I saw only two version bumping commits, one for releasing version and one for next version. I think this is what we expect. > It helps keep the version up-to-date on every build (not just releases), > > It simplifies the release profile > > It removes the use of the antrun plugin and removes the dependency tree from the build for that plugin. I am good to all above. I am positive to drop plugins if they are covered by existing plugins. I simply can't find alternatives from existing plugins to replace file contents inplace and don't think it is worth for above to change c client development experience. > (I also included a bugfix for JMockIt in https://github.com/apache/zookeeper/commit/2931f537c97cf6397f9e72f04edd5dda4b56da97) I think we can solved it separately (just like ZOOKEEPER-4928, thank you for this!). Not sure why master pass this. -- 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
