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

Mavenize the build process - Phase 1: building the native code via Maven #582

Closed

Conversation

lgoldstein
Copy link
Contributor

My goal is to eventually execute the build process using only Maven (with some help from the antrun plugin). However, Maven-izing the whole project at once is a complex and daunting task. This why, I would like to tackle it in phases, where each phase will add another Maven "conversion" of the full build process until we can achieve a full build and release cycle using only Maven. For the time being, ANT will continue to be the build framework until this goal is reached, and (as you will note) the Maven build artifacts reside under the target sub-folder, whereas the ANT ones reside under the build one. This way, we can keep the "parallel" builds without interfering with one another until we are ready to switch to the Maven one.

@lgoldstein
Copy link
Contributor Author

The component can be easily tested by:

cd native
mvn clean install

@twall
Copy link
Contributor

twall commented Jan 19, 2016

Could you please point at a good example of an "ideal" maven build for reference?

@lgoldstein
Copy link
Contributor Author

I don't know what "ideal" means, but IMO the current ANT script used to build the project is quite complex. There is a lot of "code" that Maven does better and in a more concise way. In addition to this, ultimately we want to publish the artifacts as Maven ones - therefore, using Maven to build the project seems more natural. Furthermore, the dependencies that we need (e.g., junit) are easier to manage via Maven instead of having to keep them checked in Git. As far as a Maven reference - see Apache Mina SSHD (which is also maintained by me...)

@dblock
Copy link
Member

dblock commented Jan 19, 2016

I vote for maven, however I don't think we should merge maven changes until they replace at last a big part of the build process AND simplify it. I mean I wouldn't put to on master until we rely on it to build.

@lgoldstein
Copy link
Contributor Author

As I have previously stated, I believe Maven-izing the whole project all at once is a big undertaking involving a lot (!) of changes, so doing it all at once seems very lengthy and cumbersome. This is especially true since due to the duration it may take until a stable Maven build is likely to be very long (I cannot devote 100% of my time to it...) and keeping up with the frequent changes in the master is going to be very difficult. That is why I prefer doing it in (very) small steps that we can test separately - like Lego building blockx - where at the end we will tie together all these small blocks into the big construction. Meanwhile, we keep using the ANT build until we are reasonably sure that Maven can take over. That is why these changes should reside in the master even though they are not currently being used, so that they evolve alongside the other changes.

@@ -0,0 +1,57 @@
<?xml version="1.0" encoding="UTF-8"?>
<!-- Super POM included by all other sub-POM(s)-->
Copy link
Contributor

Choose a reason for hiding this comment

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

minor: Might want to remove this comment. I'm guessing it was copy/paste from parent pom.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Will do...

@lgoldstein
Copy link
Contributor Author

Fixed all indicated remarks and re-pushed


<dependencyManagement>
<dependencies>
<dependency>
Copy link
Contributor

Choose a reason for hiding this comment

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

I don't think these ant dependencies are needed because they are not used as dependencies. Further down, they are declared as deps of a plugin in <plugin-mgt> section, which should suffice.

<dependency>
<groupId>org.mockito</groupId>
<artifactId>mockito-all</artifactId>
<version>1.10.19</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

mockito same as junit: delete until actually needed, or add <scope>test</scope>

@lgoldstein
Copy link
Contributor Author

I agree with you that the POM should not contain definitions that we don't need, so I have trimmed down some plugins and dependencies that we are not likely to need when we switch to the full Maven build. However, I prefer leaving in the definitions of the ones we will need down the road (e.g., surefire, gmaven, junit, mockito) since there are quite a few of them and they are somewhat inter-related, so it would be easy to miss adding one of them or configure them correctly.

@bhamail
Copy link
Contributor

bhamail commented Jan 25, 2016

If the poms only contain the minimum definitions needed, and if new maven build features are added incrementally via feature branches, then I think you can add definitions as needed - and avoid brittle builds.
Besides keeping things as simple as possible, this minimal approach also helps people understand "why" a given new pom configuration is added. Adding in things that are not in use/not needed has the opposite effect - it is very hard to know exactly "why" something is in there, and the change log doesn't help explain why either.

@lgoldstein
Copy link
Contributor Author

You make a good point - I'll see how I can trim this further - it will take some time though since I am going through a bit of an upheaval and cannot devote to it the time I would have liked...

</developers>

<build>
<defaultGoal>install</defaultGoal>
Copy link
Contributor

Choose a reason for hiding this comment

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

Would not recommend a defaultGoal of "install". I've seen maven projects get into trouble when accidentally requiring an "install" phase, when the "package" phase should be sufficient. I suggest either remove the defaultGoal, or change it to "package".

PS: I'm working on a PR from your branch with a bunch of the suggested changes

@bhamail
Copy link
Contributor

bhamail commented Feb 6, 2016

I pushed a new branch (based on your branch in lgoldstein) to my own fork with the "shrunken parent pom" changes. Here's the commit with the diff: bhamail@dfc729c

My git/github foo is lacking, and I couldn't figure out how to create a new PR based on your PR.
The changes are in my fork branch: maven-phase1-native-dan -> https://github.com/bhamail/jna/tree/maven-phase1-native-dan

@lgoldstein
Copy link
Contributor Author

Looks fine, although for the maven-antrun-plugin I strongly recommend adding at least the following dependencies in order to ensure that we control the exact ANT version that is being used

                        <dependency>
                            <groupId>org.apache.ant</groupId>
                            <artifactId>ant</artifactId>
                            <version>${ant.version}</version>
                        </dependency>
                        <dependency>
                            <groupId>org.apache.ant</groupId>
                            <artifactId>ant-launcher</artifactId>
                            <version>${ant.version}</version>
                        </dependency>

@bhamail
Copy link
Contributor

bhamail commented Feb 8, 2016

Re: maven-antrun-plugin -> ant version.

If we need to use the latest version of ant (though I don't think we do), just adding the first dependency (org.apache.ant:ant) should suffice - because org.apache.ant:ant declares a dependency on the same version of org.apache.ant:ant-launcher.

If we don't need to force a newer ant version than the default ant version used by maven-antrun-plugin, then I would not add the additional config now. We can easily add such config later if needed.

maven-antrun-plugin -> 1.8 declares a dependency on ant -> 1.9.4 Have you experienced a problem building with that ant version?

@lgoldstein
Copy link
Contributor Author

I haven't experienced any specific problem with 1.9.4, but why not use latest 1.9.6 - after all, there is a reason it was released (bug fixes, new features, etc...). I have been using 1.9.6 for quite a while and I am very satisfied with its stability and performance. As far as relying on the dependency declared by the antrun plugin - better to have the used ANT version stated explicitly in the POM, so that if we upgrade the antrun plugin version we do not drag in some unknown dependency for some new or un-tested ANY version.

@bhamail
Copy link
Contributor

bhamail commented Feb 9, 2016

Using the latest ANT is fine by me. In that case, we should drop the 'org.apache.ant:ant-launcher' dependency.

@lgoldstein
Copy link
Contributor Author

Just keep the ant one as it will bring in the launcher dependency with the same version. Sounds good enough - just go ahead and do it.

@bhamail
Copy link
Contributor

bhamail commented Feb 13, 2016

I pushed the explicit ant dep, and a few other changes to my branch: https://github.com/bhamail/jna/tree/maven-phase1-native-dan.

I'll try to resolve the conflicts with master next (I will mess with my branch to be safe).

PS: After this get's merged to master, I'd like to run the pom.xml files through "tidy:pom".

@bhamail
Copy link
Contributor

bhamail commented Feb 17, 2016

Fixed by PR #597

@bhamail bhamail closed this Feb 17, 2016
@lgoldstein lgoldstein deleted the maven-phase1-native branch February 21, 2016 15:33
mstyura pushed a commit to mstyura/jna that referenced this pull request Sep 9, 2024
…ive-access#573)" (java-native-access#582)

Motivation:

The used github action does not work anymore

Modifications:

This reverts commit 776c601.

Result:

Build pass again
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants