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

chore: bump to Java 11 #1190

Merged
merged 15 commits into from
Mar 8, 2021
Merged

Conversation

andre15silva
Copy link
Contributor

Closes #1186

@andre15silva andre15silva force-pushed the bump-java-11 branch 3 times, most recently from 914a77e to ecfa3c5 Compare February 15, 2021 00:40
Copy link
Contributor

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Very nice, very important PR, thanks for it.

  • see comments
  • Why is CI failing?

Jenkinsfile Outdated
@@ -8,7 +8,7 @@ kind: Pod
spec:
containers:
- name: maven
image: repairnator/ci-env:latest
image: andre15silva/repairnator-ci:latest
Copy link
Contributor

Choose a reason for hiding this comment

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

added you to https://hub.docker.com/orgs/repairnator such that you can push a new version to the organization instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ok, thanks! Will do that.

@@ -58,7 +59,8 @@ public void tearDown() throws IOException {
FileHelper.deleteFile(tmpDir);
}

@Test
@Ignore
//@Test
Copy link
Contributor

Choose a reason for hiding this comment

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

avoid comments + @ignore is enough. could you add a comment to explain why it is ignored?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Only @Ignore didn't work for some reason. I commented @Test just to make it skip. I'll debug that.

@andre15silva
Copy link
Contributor Author

* Why is CI failing?

I'm debugging that. Just opened this PR to have the CI env running to check in parallel.

@monperrus
Copy link
Contributor

Now that we fixed the Sorald issue, what about finishing the move to Java 11?

@andre15silva
Copy link
Contributor Author

Now that we fixed the Sorald issue, what about finishing the move to Java 11?

Yup, I'm gonna work on that this afternoon. Hopefully it will be finished today.

@andre15silva andre15silva force-pushed the bump-java-11 branch 5 times, most recently from cf99e8e to 05559cc Compare February 18, 2021 19:54
@andre15silva
Copy link
Contributor Author

andre15silva commented Feb 18, 2021

We need to deploy a new version of maven-repair, that supports Java 11, before upgrading the pipeline, as NPERepair depends on it still (changes with #1172).
This is the major cause of failures, not being able to access the new version.
I have tried using the version that is installed during the build, but this makes other tests break, as I change the local repository from the project directory to the home directory.

Also, what does TestGlobalPatchAnalysis.testODSPatchClassification do exactly? Is it only related to Nopol? It is failing because Nopol requires Java 8.

Same thing goes for TestProjectInspector.testPatchFailingProject, but I assume we can just remove Nopol from the repair tools for that test.

@andre15silva
Copy link
Contributor Author

@monperrus do you think we can deploy maven-repair as it is in this PR, so that we can use that version as a dependency in the pipeline?

@javierron
Copy link
Contributor

javierron commented Feb 22, 2021

Hey @andre15silva

Also, what does TestGlobalPatchAnalysis.testODSPatchClassification do exactly? Is it only related to Nopol? It is failing because Nopol requires Java 8.

I think that the goal of that test is to check that ODS classification is applied to the generated patches, regardless of the tool used to generate the patches, so replacing Nopol for other tool is not a problem.

Same thing goes for TestProjectInspector.testPatchFailingProject, but I assume we can just remove Nopol from the repair tools for that test.

The same applies here, the goal of the test seems to be to check that all steps, serializers, etc. are correct. However to remove Nopol from both tests we would have to provide a tool/project combination that creates a patch (maybe introducing a NPE in the test project?).

Also, I have a question: Is the goal to move the project to a java 11 only version? If that is true why not bump or remove Nopol completely? Or do we want to keep the project compatible with both java 8 and 11 simultaneously?

Please tell me if I can help on something to get this merged, I would like to rebase my work to this, to avoid any possible java version issues.

@monperrus
Copy link
Contributor

@monperrus do you think we can deploy maven-repair

Do you mean before this PR? If "yes", I'd be happy to redeploy the latest version.

@andre15silva
Copy link
Contributor Author

Do you mean before this PR? If "yes", I'd be happy to redeploy the latest version.

Yes. If you can deploy the version before this PR with the profile noNopol activated that would be great. Or as it sits in this PR, which is with that profile active by default (and Nopol tests deactivated).

@andre15silva
Copy link
Contributor Author

andre15silva commented Feb 22, 2021

Hey @andre15silva

Hi @javierron !

I think that the goal of that test is to check that ODS classification is applied to the generated patches, regardless of the tool used to generate the patches, so replacing Nopol for other tool is not a problem.

I see, that's nice. Do you know where I can find documentation/information on ODS classification? I'm not being able to find it.

The same applies here, the goal of the test seems to be to check that all steps, serializers, etc. are correct. However to remove Nopol from both tests we would have to provide a tool/project combination that creates a patch (maybe introducing a NPE in the test project?).

I'll try removing. I think it should be able to find a patch even without Nopol, otherwise I'll just change the project or move it to another build.

Also, I have a question: Is the goal to move the project to a java 11 only version? If that is true why not bump or remove Nopol completely? Or do we want to keep the project compatible with both java 8 and 11 simultaneously?

If I'm not mistaken, the goal is to drop support for Java 8. @monperrus what do you think should be the approach to Nopol? Upgrading it or removing it from Repairnator.

Please tell me if I can help on something to get this merged, I would like to rebase my work to this, to avoid any possible java version issues.

Thanks, I'll ping you if there's anything that you can help with. Apart from the maven-repair and Nopol issues, I don't think there's anything else right now.

@javierron
Copy link
Contributor

Do you know where I can find documentation/information on ODS classification?

This is part of the Coming repository, you can check the docs here

@andre15silva
Copy link
Contributor Author

This is part of the Coming repository, you can check the docs here

Thanks!

@monperrus
Copy link
Contributor

If I'm not mistaken, the goal is to drop support for Java 8

yes!

What remains to be done here in order to merge?

@andre15silva
Copy link
Contributor Author

What remains to be done here in order to merge?

The deployment of maven-repair with noNopol profile activated. Without that the pipeline can't use NPEFix.

@monperrus
Copy link
Contributor

Just tried to deploy maven-repair.

It fails, because maven-repair does not compile

$ mvn clean deploy -PnoNopol
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/martin/martin-no-backup/repairnator/src/maven-repair/src/main/java/com/github/tdurieux/repair/maven/plugin/NopolMojo.java:[30,1] cannot find symbol
  symbol:   static checkToolsJar
  location: class
[ERROR] /home/martin/martin-no-backup/repairnator/src/maven-repair/src/main/java/com/github/tdurieux/repair/maven/plugin/NopolMojo.java:[62,13] cannot find symbol
  symbol:   method checkToolsJar()
  location: class com.github.tdurieux.repair.maven.plugin.NopolMojo

Are you able to compile the latest master of maven-repair with -PnoNopol? How can a compilation failure happen given that we have CI?

Thanks!

@andre15silva
Copy link
Contributor Author

Just tried to deploy maven-repair.

It fails, because maven-repair does not compile

$ mvn clean deploy -PnoNopol
[ERROR] COMPILATION ERROR : 
[INFO] -------------------------------------------------------------
[ERROR] /home/martin/martin-no-backup/repairnator/src/maven-repair/src/main/java/com/github/tdurieux/repair/maven/plugin/NopolMojo.java:[30,1] cannot find symbol
  symbol:   static checkToolsJar
  location: class
[ERROR] /home/martin/martin-no-backup/repairnator/src/maven-repair/src/main/java/com/github/tdurieux/repair/maven/plugin/NopolMojo.java:[62,13] cannot find symbol
  symbol:   method checkToolsJar()
  location: class com.github.tdurieux.repair.maven.plugin.NopolMojo

Are you able to compile the latest master of maven-repair with -PnoNopol? How can a compilation failure happen given that we have CI?

mvn clean compile -PnoNopol works for me.

Did you install repairnator-core before? That static method checkToolsJar is in repairnator-core.

@monperrus
Copy link
Contributor

You're right. The latest snapshot of core was from November (because CD is broken, see #1167 )

Delivered a new core 3-3-SNAPSHOT manually.

Just deployed version 0bf3bd6ffadf3638ee604b83053426c739aff1f7 to Maven Central. Note that it depends on a snapshot (3-3-SNAPSHOT) which is a bad practice (releases should never depend on snapshots).

But for now, we can proceed with 0bf3bd6ffadf3638ee604b83053426c739aff1f7, which will soon appear at https://repo.maven.apache.org/maven2/fr/inria/gforge/spirals/repair-maven-plugin/

@andre15silva
Copy link
Contributor Author

Thanks! I'll get this working with that version.

Note that this is just temporary, when #1172 is merged it will no longer be necessary.

@monperrus
Copy link
Contributor

great, this move to Java 11 will be a serious milestone!

@andre15silva
Copy link
Contributor Author

Is it normal to take this long to appear in the repository?

@monperrus
Copy link
Contributor

release at last successful: https://repo1.maven.org/maven2/fr/inria/gforge/spirals/repair-maven-plugin/0bf3bd6ffadf3638ee604b83053426c739aff1f7/

@andre15silva
Copy link
Contributor Author

andre15silva commented Mar 1, 2021

There is now a problem with gzoltar.

When using mvn fr.inria.gforge.spirals:repair-maven-plugin:0bf3bd6ffadf3638ee604b83053426c739aff1f7:npefix on a project, we get this error:

[DEBUG] Using transporter WagonTransporter with priority -1.0 for http://sachaproject.gforge.inria.fr/repositories/releases/
[DEBUG] Using connector BasicRepositoryConnector with priority 0.0 for http://sachaproject.gforge.inria.fr/repositories/releases/
Downloading from gforge.inria.fr-release: http://sachaproject.gforge.inria.fr/repositories/releases/com/gzoltar/gzoltar/0.1.1/gzoltar-0.1.1.pom
Progress (1): gzoltar-0.1.1.pom (462 B)
[WARNING] Could not validate integrity of download from http://sachaproject.gforge.inria.fr/repositories/releases/com/gzoltar/gzoltar/0.1.1/gzoltar-0.1.1.pom
org.eclipse.aether.transfer.ChecksumFailureException: Checksum validation failed, no checksums available

This happens because there is no checksum available for this version of gzoltar on this repository, and maven central does not have this version available to serve as backup.
This doesn't show up when installing maven-repair tho, it doesn't check it for some reason I haven't figured out.

I see three ways of fixing this:

  • Moving gzoltar dependency to a more recent version that is available in maven central, and then re-deploying maven-repair. I don't know why we are using this specific version tho.
  • Updating the gzoltar deploy so it has the checksums.
  • Merging refactor: refactor the NPEFix pipeline #1172 before, so that the pipeline does all the work and doesn't need to call maven-repair.

I think the later two are probably the better options.

@monperrus WDYT?

@andre15silva
Copy link
Contributor Author

I would prefer to merge #1172 first then. It looks good overall and CI is green. What remains to be done there?

If we set aside the scope issue as discussed, it is ready to be merged.

@andre15silva andre15silva force-pushed the bump-java-11 branch 2 times, most recently from 86279bf to 14110c1 Compare March 4, 2021 11:18
@andre15silva andre15silva force-pushed the bump-java-11 branch 2 times, most recently from 9c6dfd4 to cfa8370 Compare March 5, 2021 16:43
@andre15silva
Copy link
Contributor Author

Changelog:

  • Move CI docker image to Java 11
  • Activate noNopol profile by default on maven-repair and repairnator-pipeline so that we don't load tools.jar
  • Change synthesis and localizer default params to SMT and CoCoSpoon respectively, so as to support Java 11.
  • Add z3 binaries to test resources in maven-repair to enable SMT
  • Update maven-jacoco-plugin to a Java 11 compatible version
  • Change gitrepourl of AstorJKali tests to the same project but compatible with Java 11
  • Update test assertions due to changes in Java version and Nopol parameters

@andre15silva andre15silva marked this pull request as ready for review March 5, 2021 21:27
@andre15silva
Copy link
Contributor Author

Ready for review! @monperrus

Copy link
Contributor

@monperrus monperrus left a comment

Choose a reason for hiding this comment

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

Waoow, it looks really good @andre15silva ! See my last comments about the testing infrastructure and then we're ready to officially move to Java 11.

@@ -72,7 +72,7 @@ public void testPipelineOnlyGitRepository() throws Exception {
public void testPipelineGitRepositoryAndBranch() throws Exception {
GitRepositoryLauncher l = new GitRepositoryLauncher(new String[]{
"--gitrepo",
"--gitrepourl", "https://github.com/surli/failingProject",
Copy link
Contributor

Choose a reason for hiding this comment

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

What about using https://github.com/repairnator/failingProject/ instead? (sent you an invitation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@@ -52,7 +52,7 @@ public void testPipelineOnlyGitRepository() throws Exception {
public void testPipelineGitRepositoryAndBranch() throws Exception {
GithubMainProcess mainProc = (GithubMainProcess) MainProcessFactory.getGithubMainProcess(new String[]{
"--gitrepo",
"--gitrepourl", "https://github.com/surli/failingProject",
"--gitrepourl", "https://github.com/andre15silva/failingProject",
Copy link
Contributor

Choose a reason for hiding this comment

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

idem

@@ -134,7 +135,7 @@ protected void runNopol(Set<FailureLocation> failureLocation, List<String> tests
nopolContext.setMaxTimeInMinutes(timeout);
nopolContext.setLocalizer(NopolContext.NopolLocalizer.COCOSPOON);
nopolContext.setSolverPath(this.getConfig().getZ3solverPath());
nopolContext.setSynthesis(NopolContext.NopolSynthesis.DYNAMOTH);
Copy link
Contributor

Choose a reason for hiding this comment

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

could you add a comment to explain why we cannot use DYNAMOTH?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done!

@andre15silva
Copy link
Contributor Author

Ready after CI runs again

@monperrus monperrus changed the title Prepare for Java 11 chore: bump to Java 11 Mar 8, 2021
@monperrus monperrus merged commit ac61fab into eclipse-repairnator:master Mar 8, 2021
@monperrus
Copy link
Contributor

Thanks a lot @andre15silva for this foundational contribution.

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.

Bump Java version to Java 11
3 participants