Remove maven-antrun-plugin

[ctubbsii]

Replace maven-antrun-plugin with better alternatives:

• Use build-helper-maven-plugin for keeping versions up-to-date in cclient code
• Use build-helper-maven-plugin for retrieving hostname
• Use Maven native env.* properties instead of antrun
• Simplify maven-release-plugin to remove extra commit (versions should already be up-to-date from previous SNAPSHOT builds during development)
• Use exec-maven-plugin for calling make
• Use maven-resources-plugin for creating empty working directory
• Use maven-resources-plugin for injecting the version into the cclient files (without the -SNAPSHOT suffix)
• [BUGFIX] Fixes the incorrect pattern matching of version in zookeeper_version.h

Additionally, perform some minor cleanup of modified files:

• Fix xml indentations in modified pom.xml files
• Use ${project.build.directory} property instead of target when possible
• [BUGFIX] Fix some paths that used semi-colons as the path separator and use the OS-dependent path separator

This fixes a few bugs (see above notes).

[ctubbsii]

This change does not tag a specific JIRA issue... there were two bugs fixed in this, as a side-effect of the overall goal. I'm not sure how the ZooKeeper committers prefer to tag something like this.

[ctubbsii]

It looks like the java fail might be related to low memory? It's not clear to me yet. I didn't modify any Java files in this PR.

[ctubbsii]

Also, while the cppunit tests are passing in GitHub Actions, they fail for me locally, always in the same place:

./zookeeper-client/zookeeper-client-c/tests/TestReadOnlyClient.cc:118: Assertion: equality assertion failed [Expected: 0, Actual  : -15]
./zookeeper-client/zookeeper-client-c/tests/TestClient.cc:890: Assertion: equality assertion failed [Expected: 0, Actual  : -15]
Failures !!!
Run: 84   Failure total: 2   Failures: 2   Errors: 0

I'm not sure what the problem could be or how to track down the errors further.

[kezhuw]

@ctubbsii Thank you for doing this! I am positive to drop one plugin.

It would be nice to generate only version related parts for CMakeLists.txt, configure.ac to include.

It looks like the java fail might be related to low memory?

It happens locally in my env.

[kezhuw]

Ignore this file in .gitignore ?

[ctubbsii]

No, these shouldn't change in the build. They are copied into place with substitutions. Those files that are changed are already tracked.

My preference would be to have the build set up something in /target for the c build to work out of... that way all the Java build stuff can modify these files in the target directory, rather than in the src tree. But this PR was supposed to be a small change, not a big sweeping one. So, I just made templates that are checked in to automatically modify the ones that they edit that are already checked in.

[kezhuw]

My preference would be to have the build set up something in /target for the c build to work out of... that way all the Java build stuff can modify these files in the target directory, rather than in the src tree. But this PR was supposed to be a small change, not a big sweeping one.

I support all of above, but the result of this pr make things worse from my point of view.

Before this pr, the thing is that those files are tracked in place and we need one compilation phase to change their content.

After this pr, these files are replaced in compilation and people have to filter out sources of these files to make change to take effect. This costs extra efforts.

I would like to limit these source files to be only version related so we have no chance to touch them.

Those files that are changed are already tracked.

If those files are only version related, I don't see much of value to be tracked. But I am ok to both, tracked or not.

[ctubbsii]

@kezhuw 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. If we stop tracking the generated file, or remove it, devs will be required to run Maven first before running outside Maven. Is that okay? Is that better?

Currently, they only need to run the Maven build to recreate those files if the version changes, or if other changes are made in the source templates I made in src/main/c-filtered/; if we remove these files and stop tracking them, then dev will be required to always run Maven first.

Would a good compromise be to add a comment to the files saying this: This is a generated file. If you are a developer, make changes to the template file in src/main/c-filtered ? Or is it still better to just remove the files and stop tracking them? (and maybe also add the comment)

[ctubbsii]

@kezhuw Does b52ad0a address your concerns?

[kezhuw]

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. If we stop tracking the generated file, or remove it, devs will be required to run Maven first before running outside Maven. Is that okay? Is that better?

Thank you for point out this! I think it is a good to make build of c client independent. I found installation guide for c binding. I would say it is good and let's don't change it.

Currently, this pr require additional maven build for changes in cmake/automake files. I don't think it is worth for us to go this direction.

Does b52ad0a address your concerns?

Sorry for my rollback! I don't think the extra maven build is good for c client.

[ctubbsii]

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.

[kezhuw]

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?

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 2931f53)

I think we can solved it separately (just like ZOOKEEPER-4928, thank you for this!). Not sure why master pass this.

[ctubbsii]

I tracked down one Java build problem, and it seems to be with the zookeeper-server module's use of jmockit. The configuration was done incorrectly. I'm not sure why it seems to work in the master branch, or why it still works in JDK8, but it seems to be completely broken. I fixed it here. However, one issue in future is that JMockIt doesn't support JDK17, so ZooKeeper is probably going to have to figure out a solution to get rid of it soon anyway.