Skip to content
This repository has been archived by the owner on Mar 17, 2021. It is now read-only.

Conversation

davidfestal
Copy link
Collaborator

Description of the problem

The RH distribution build has several strong requirements such as:

  • Producing several variants of the RH Che distribution with a single build, while ensuring that the build process and intermediate artifacts of each variant are fully isolated,
  • Deriving the version number of the produced RH Che distribution assembly artifacts (for all variants) from the version of the upstream Che,
  • Reusing ( and modifying / completing ) both the source and the generated artifacts of some upstream Che maven modules (e.g. the assembly-ide-war GWT application) to provide the variants of the RH Che distribution artifacts,
  • Checking out the a given branch of the upstream Che version and building it when the build is used by CI.

To meet these requirements, the maven build has a specific structure (using the maven invoker plugin) that, until now, was not compatible with the M2Eclipse integration. This prevented using JDT services (completion, cross-references, etc ...) on modules that are also Java projects (such as the plugins or the assembly-wsmaster-war)

Description of the Fix

This PR introduces slight changes that allow importing the RH distribution maven sub-modules as Maven projects in Eclipse. For projects that are Java projects, M2Eclipse can now correctly calculate the classpath based on the Maven POM file, and all the services of JDT are available again.
However the main structure of the build is not impacted, so that it doesn't impact / break the requirements described above.

Signed-off-by: David Festal dfestal@redhat.com

@davidfestal davidfestal self-assigned this Jun 2, 2017
@davidfestal davidfestal requested a review from l0rd June 2, 2017 08:01
@davidfestal
Copy link
Collaborator Author

@mario could you review this PR please ?

@l0rd
Copy link
Contributor

l0rd commented Jun 2, 2017

@davidfestal I think that we should review the list of strong requirements. I'm not sure we really need all of them.

By the way I'm ok with this PR. Before merging I would like @ibuziuk and @sunix to have a look at that because I think they asked for this change.

@davidfestal
Copy link
Collaborator Author

davidfestal commented Jun 2, 2017

I think that we should review the list of strong requirements. I'm not sure we really need all of them.

Sure, I should have said strong initial requirements. as soon as we are based on the upstream master and can have access to their snapshot Maven repository, point 3 and 4 wouldn't be strong constraints anymore.

However requirements 1 or 2 are still relevant as far as I understand, and are really met by the current build structure.

cc @sunix , cc @ibuziuk

@ibuziuk ibuziuk self-requested a review June 2, 2017 14:55
@davidfestal davidfestal requested a review from sunix June 2, 2017 15:23
@davidfestal
Copy link
Collaborator Author

So finally I was able to also test this fix against the Che maven support (in OSIO):
On a centos-rhche-in-che workspace (on a stack I just created) on OSIO, I was able to convert one of the RH build sub-modules (keycloak-plugin-ide) to a Java/Maven project and have the classpath correctly filled and the Java completion + error markers working.

@amisevsk
Copy link
Collaborator

amisevsk commented Jun 2, 2017

I've tested importing into eclipse, and it works great! Thanks @davidfestal

Copy link
Member

@ibuziuk ibuziuk left a comment

Choose a reason for hiding this comment

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

@davidfestal great job! makes development much more easier

<groupId>com.google.inject</groupId>
<artifactId>guice</artifactId>
</dependency>
<dependency>
<groupId>aopalliance</groupId>
Copy link
Contributor

Choose a reason for hiding this comment

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

Not sure this is related to the fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

This is related to a pom sorting error I encountered during the work on this PR.

@@ -10,7 +10,7 @@
<parent>
<artifactId>che-parent</artifactId>
<groupId>org.eclipse.che</groupId>
<version>@che.version@</version>
Copy link
Contributor

Choose a reason for hiding this comment

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

because of that, I prefere this PR to be merged/adapted after #89

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Do you have any idea when you will be able to merge #89 ? it seems it depends on the merge of the openshift-connector-rebased branch of eclipse/che, which might take some more time.

What is the exact problem with this line ? Is it the fact that, on your openshift-connector-rebased branch, you will need to change the 5.6.0-openshift-connector-SNAPSHOT version to make it consistent with the upstream master version (which you already did for the root pom.xml here ?

Copy link
Contributor

Choose a reason for hiding this comment

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

Yes

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

OK, so what is the risk or complexity ? Just one line to change in your existing PR, or am I missing something ?

Anyway, any idea about when you should be able to merge #89 ?
It would be useful for developers to have this PR merged while working on the remaining subtasks of CHE-229 in the meantime.

Copy link
Member

Choose a reason for hiding this comment

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

< It would be useful for developers to have this PR merged while working on the remaining
+1, without David's fix it's utterly difficult to develop plugins for rh-che

<configuration>
<sortProperties>false</sortProperties>
</configuration>
</plugin>
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure this is related to the fix

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

it is

Signed-off-by: David Festal <dfestal@redhat.com>
@davidfestal davidfestal force-pushed the Make_Maven_Projects_Importable_In_Eclipse branch from 0098711 to 9572060 Compare June 23, 2017 12:06
@davidfestal davidfestal changed the base branch from master to openshift-connector-rebased June 23, 2017 12:07
@davidfestal
Copy link
Collaborator Author

@sunix let me merge this in openshift-connector-rebased as we discussed this morning.

@davidfestal davidfestal merged commit 2dcddbd into redhat-developer:openshift-connector-rebased Jun 23, 2017
davidfestal added a commit that referenced this pull request Jun 26, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
sunix pushed a commit that referenced this pull request Jun 28, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
amisevsk added a commit to amisevsk/rh-che that referenced this pull request Jun 28, 2017
amisevsk added a commit to amisevsk/rh-che that referenced this pull request Jun 30, 2017
benoitf pushed a commit to benoitf/rh-che that referenced this pull request Jul 3, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
sunix pushed a commit that referenced this pull request Jul 4, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
benoitf pushed a commit to benoitf/rh-che that referenced this pull request Jul 7, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
sunix pushed a commit that referenced this pull request Jul 12, 2017
Signed-off-by: David Festal <dfestal@redhat.com>
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants