search

google / certificate-transparency

Auditing for TLS certificates.
https://certificate.transparency.dev
Apache License 2.0
869 stars 283 forks source link

Use maven to build java code #1375

Closed alexanderkjall closed 7 years ago

alexanderkjall commented 7 years ago

Some remarks:

The .idea directory is deleted and added to .gitignore, please backup your private copy of it before you pull this in order to not loose any settings.

The /test/testdata directory is symlinked in java/src/test/resources. This works fine on my linux machine, but might cause trouble with windows maybe? I don't really have any experience with windows.

I have signed the CLA.

Should I maybe collapse the commits?

feedback is welcome :)

pphaneuf commented 7 years ago

If there's going to be some work on the Java code (which is quite welcome!), could I ask for a temporary "freeze" after this PR is merged, to do some repository reorganisation? It would likely break PRs that are "in flight" for this area, but I think it would make things easier for you after... :smiley:

codingllama commented 7 years ago

OK, I had a look at the Travis failure and the problem is that ant build test is failing, as build.xml is now outdated.

My suggestion is to kill it with fire (along with build.properties) and replace ant with a cd java && mvn test install at .travis.yml (plus adding the Maven dependency to it, doing a bit of a cleanup, etc).

@pphaneuf , any objections?

pphaneuf commented 7 years ago

If you're pulling the carpet out from under Ant, and replacing it with Maven, then it would make sense to fix up the .travis.yml to run the appropriate Maven commands there.

As long as the tests are being run (and their failure cause Travis to fail as well, which hasn't always been true!), I'm happy. :wink:

alexanderkjall commented 7 years ago

I don't have any experience with travis, so i haven't made any changes to the .travis.yml file as I didn't have any easy way to verify that it work.

Is there some way to run the travis file, so that I don't have to do an commit in order to do a testrun?

alexanderkjall commented 7 years ago

The travis build system seem to have defeated me.

This is my theory of the problem: The apt addon doesn't update it's index before it tries to download stuff, therefor it doesn't find the java-8 packages and the setting of JAVA_HOME points to an invalid path.

If i try to run "sudo apt-get update" in the before_install section, the build fails on OS X.

And i can't find a way to run apt-get update in the apt section, but maybe my googling skills are too weak.

Edit: Found out that you can specify the jdk version in the .travis.yml file, this works for mac os x but not for linux.

pphaneuf commented 7 years ago

I was hoping this wouldn't happen and it would all Just Work, but I had a feeling it could... Hence my clever wording that "all I needed to be happy" was for Travis to run all the tests properly. :wink:

I'm sure it's possible, but that stuff always needs some fiddling around... Good luck!

codingllama commented 7 years ago

Sorry about Travis, it can be a bit painful sometimes. No promises, but I'll see if I can have a go at it later today.

alexanderkjall commented 7 years ago

I have thought about the problem a bit since yesterday. And I think it boils down to that the maven package that gets installed by the apt section uses the wrong java version.

The next things I will try: 1) There might be some other way to install maven 2) Maybe it's already installed. 3) Maybe there is some way to conditionally set JAVA_HOME

codingllama commented 7 years ago

I had a JAVA_HOME mismatch when I was playing around with the Docker container. I think the box comes with java7 and sets JAVA_HOME to it, which maven respects.

Could you add a java -version and a mvn -version at the before_install section?

You could unset JAVA_HOME and see if it helps. This seemed to work in the Docker container:

travis@91be326c05ab:~$ java -version
java version "1.8.0_121"
...
travis@91be326c05ab:~$ mvn -version
Apache Maven 3.2.5 (12a6b3acb947671f09b81f49094c53f426d8cea1; 2014-12-14T17:29:23+00:00)
Maven home: /usr/local/maven
Java version: 1.7.0_76, vendor: Oracle Corporation
Java home: /usr/lib/jvm/java-7-oracle/jre
...
travis@91be326c05ab:~$ unset JAVA_HOME
travis@91be326c05ab:~$ mvn -version
Apache Maven 3.2.5 (12a6b3acb947671f09b81f49094c53f426d8cea1; 2014-12-14T17:29:23+00:00)
Maven home: /usr/local/maven
Java version: 1.8.0_121, vendor: Oracle Corporation
Java home: /usr/lib/jvm/java-8-oracle/jre
...

If the above doesn't work, you can also try to set JAVA_HOME with this monster I put together here. It should be relatively portable, but I haven't tested under OSX.

export JAVA_HOME="$(java -XshowSettings:properties -version 2>&1 | grep 'java.home' | awk -F' = ' '{print $2}')"
codingllama commented 7 years ago

On the plus side, we know the build fails when maven goes wrong, so there's that. :)

pphaneuf commented 7 years ago

That was a specific problem in the past, where I knew I had broken something, and it turned out the test command still returned a zero status? See commit 0675b70f6b4355de5650dd5e8c5cf8d25a06604e... :stuck_out_tongue:

eranmes commented 7 years ago

Out of curiosity, why the proposal to remove the .idea directory? What's the alternative?

alexanderkjall commented 7 years ago

My motivation for removing the .idea is that the pom.xml file should be the canonical way that the project gets built.

It's easy to configure intellij to use the pom.xml file when starting the project, file -> new -> Project from existing sources -> select the pom.xml file.

And also since not all user-specific settings where included in the .gitignore file I felt that it's about as much work removing it as adding more stuff to it.

codingllama commented 7 years ago

@eranmes I stand behind removing IDE-specific from the repos for a few reasons:

  1. It's not uncommon for IDE settings to have assumptions about the machine they were created or user specific settings (local paths, references to your favorite code style, so on).
  2. Maven becomes the canonical source of configuration, so there's no room for mismatch.
  3. Maven is well supported by Java IDEs (Eclipse, IntelliJ). As Alexander said, you can just open it in IntelliJ.
  4. IDE-specific settings are poor form and personal. What if someone uses Eclipse? Or vim? Or notepad?

Maven is standard in the Java community. We should make the move to it.

codingllama commented 7 years ago

I've been following your attempts at Travis, but it doesn't seen to be working for linux (java -version is 1.7 regardless of the jdk setup). I think it boils to the top-level language on the travis.yml file, but I don't want to mess with that.

We could set the project to use Java 1.7 - this way we don't need special JDKs or JAVA_HOME magic.

We can fight Travis for Java8 support on another PR, then.

pphaneuf commented 7 years ago

Could the code changes be merged in while keeping Ant for now? Because I might be able to help with this, at the cost of having to re-do this PR, and I'm thinking that the build system part of it (which is what's not working here without the correct Java version, I think?) might not be bad to re-do, so if the code changes could be salvaged (changing this PR from "use Maven" to "reorganise the Java code a bit"), it might be the easiest way?

alexanderkjall commented 7 years ago

The only code changes I made was to replace the access to the test files to be done in a more maven friendly way, and I don't think those changes will work with ant.

But I don't think there is anything in the codebase that requires java 8, I simply defaulted to that since java 7 is EOL.

I'll reduce the language level to 1.7 and make a try with that right after work.

pphaneuf commented 7 years ago

OK, makes sense. Give that a try, we'll see how it goes. Even if I do my own reorganisation, it should be possible to save this PR as a diff, and re-apply it cleanly enough after I've done my thing, but if you can get this PR to pass the tests and merge it, that'd be the best. :smiley:

alexanderkjall commented 7 years ago

Seems like i got the tests running by hard coding the java 8 path for linux into the travis file.

This feels a bit like building a house with rubber bands and glue, but maybe it's good enough?

codingllama commented 7 years ago

Btw, I think we're really close to the end here.

alexanderkjall commented 7 years ago

So... that just looks like a bug somewhere in travis? I'll just make a whitespace commit to trigger a new build and then we can see if it corrects itself. Unless you have any other suggestion.

codingllama commented 7 years ago

I didn't see what happened, but Travis is not 100% free of flakes, so there's that.

Alexander, many thanks for the PR! It's a great improvement to our Java client and I'm happy to see it get through.

alexanderkjall commented 7 years ago

At last! Thanks for all the work guiding this pull request to completion, this is one of the most professional and pleasant open source experiences I have had.

Regarding travis, I checked their incident log on their homepage, and they had an incident with a partition running out of space for the mac cluster yesterday.

pphaneuf commented 7 years ago

FYI, there is a "retry build" button somewhere in Travis, but I think you might have to be an owner of the repository (so you wouldn't have been able to trigger it, we'd have had to do it for you).