Remove eclipse conditionals #44075
Remove eclipse conditionals #44075
Conversation
We used to have some meta projects with a `-test` prefix because historically eclipse could not distinguish between test and main source-sets and could only use a single classpath. This is no longer the case for the past few Eclipse versions. This PR adds the necessary configuration to correctly categorize source folders and libraries. With this change eclipse can import projects, and the visibility rules are correct e.x. auto compete doesn't offer classes from test code or `testCompile` dependencies when editing classes in `main`. Unfortunately the cyclic dependency detection in Eclipse doesn't seem to take the difference between test and non test source sets into account, but since we are checking this in Gradle anyhow, it's safe to set to `warning` in the settings. Unfortunately there is no setting to ignore it. This might cause problems when building since Eclipse will probably not know the right order to build things in so more wirk might be necesarry.
alpar-t
added
>non-issue
:Delivery/Build
|
Pinging @elastic/es-core-infra |
build.gradle
Outdated
| // edit the generated .classpath to correctly mark test vs non test code and dependencies | ||
| // for more info: https://github.com/eclipse/buildship/issues/689 and https://github.com/gradle/gradle/issues/4802 | ||
| Set<String> testOnlyProjectDependencies = new HashSet(); | ||
| if (configurations.findByName("testCompile") != null) { |
There was a problem hiding this comment.
We should be using testCompileClasspath instead of testCompile since the later is deprecated.
build.gradle
Outdated
| Set<String> testOnlyProjectDependencies = new HashSet(); | ||
| if (configurations.findByName("testCompile") != null) { | ||
| testOnlyProjectDependencies.addAll( | ||
| configurations.testCompile.getDependencies().findAll { |
There was a problem hiding this comment.
We should use getAllDependencies() here to ensure we also get dependencies from parent configurations (like compileOnly). Also we should stick to Groovy conventions and use property accessors allDependencies vs getter methods getAllDependencies() just for consistency. I realize this is difficult switching between Java and Groovy a lot 😄
build.gradle
Outdated
| it.entryAttributes['test'] = (it.entryAttributes['gradle_used_by_scope'] == 'test') | ||
| } | ||
| if (it.hasProperty("path") && it.path.startsWith("/:")) { | ||
| if (testOnlyProjectDependencies.contains(it.path.substring(1))) { |
There was a problem hiding this comment.
Can we ditch the extra nested if for just another conditional?
libs/core/.project1
Outdated
| @@ -0,0 +1,17 @@ | |||
| <?xml version="1.0" encoding="UTF-8"?> | |||
There was a problem hiding this comment.
I think this was mistakenly checked in.
| @@ -68,7 +68,9 @@ dependencies { | |||
| hdfsFixture project(':test:fixtures:hdfs-fixture') | |||
| // Set the keytab files in the classpath so that we can access them from test code without the security manager | |||
| // freaking out. | |||
| testRuntime files(project(':test:fixtures:krb5kdc-fixture').ext.krb5Keytabs("hdfs","hdfs_hdfs.build.elastic.co.keytab").parent) | |||
| if (isEclipse == false) { | |||
There was a problem hiding this comment.
Why is this suddenly needed?
| @@ -62,6 +62,7 @@ public static String toString(Object value) { | |||
| return "null"; | |||
| } | |||
|
|
|||
|
|
|||
|
@atorok thanks for much for tackling this. Fyi I tried rebuilding my complete workspace with this but unfortunately there were quite a number of errors. Most prominently most modules/plugins are missing dependencies to some Lucene jars and some to |
|
For the record I'm seeing the same issue as Christoph. Note that the test folder is seeing the Lucene and logging JARs as expected, only the main folder is not seeing them. |
|
I'm working with @cbuescher to set this up. For anyone testing this, since there are projects being removed ( all the The usual method of Would only work if done before checking out the PR. |
|
After looking at the build problems some more and doing a small sample project it became apparent that eclipse can't handle our setup even when source folders are correctly marked for tests and non test code. The circular dependency is not really circular with this considered. There is however a solution to this problem: Buildship - the Gradle integration from the Eclipse side. IDEA has been working like this for a long time and it's a much better maintained solution than the Gradle eclipse project that we use to generate the configuration so far. This does mean that we would only support Eclipse with build ship. To try this out run then Import using Existing Gradle Project Also note that I removed the workaround to mark source folders and libraries as tests, |
|
I'll update |
I don't see this as a problem. This is the official way to use Gradle with Eclipse and is favored over the legacy
So I assume we'd need to wait for the 5.6 release to merge this? |
First try was partially successful at least (using newest Eclipse 2019-06). I had to do a few things though:
My biggest issue atm is that I cannot run unit test, e.g. in the "server" project, like I was able to do before. As an experiment I tried running "IdsQueryBuilderTests" from the IDE and got Jar-Hell errors for the test classpath: Which might be cause by the circular dependencies I was supressing earlier on, no idea really. |
| // it adds to the classpath | ||
| project.file("$buildDir/pluginUnderTestMetadata").mkdirs() | ||
| } | ||
| eclipse.classpath.defaultOutputDir = file('build-eclipse') |
There was a problem hiding this comment.
Do we still need this? I'm not completely sure what issue this was originally meant to workaround but I think with Buildship it would behoove us to have Gradle and Eclipse share build directories.
There was a problem hiding this comment.
That would have been my expectation as well, but without it Eclipse still creates the bin directories even with build-ship.
|
@mark-vieira I don't think we need to wait for |
|
I ran a re-import of the master branch in a new workspace with the last two commits and didn't run into any intial trouble. Unit tests (at least the ones I most frequently run in :server) work now. I still get the weird "Groovy:Access" error in build-tools, but ignore that as sth. cosmetic atm by closing the project. |
|
NOTE: When trying this out for the first time, a full clean with and a re-import is needed ( since projects are being removed ). |
|
@elasticmachine run elasticsearch-ci/bwc |
1 similar comment
|
@elasticmachine run elasticsearch-ci/bwc |
|
@elasticmachine run elasticsearch-ci/bwc |
|
Merged master in and made the necessary changes to make it work again. Note that I had some errors shown initially and they just went away without me doing anything was a bit wired but I think it's a glitch until Eclipse gets to build all the classes some class not found errors are shown. Please test again as time permits. My feeling from the slack discussions is that it's ok to move forward with this. |
|
@elasticmachine run elasticsearch-ci/2 run elasticsearch-ci/bwc and run elasticsearch-ci/oss-distro-docs |
There was a problem hiding this comment.
LGTM.
My only concern is the need to run ./gradlew eclipse before importing the projects using gradle buildship to get the setting to only warn on circular dependencies. The reason for my concern here is presumably if we need to set a different similar setting in the future this will also not get picked up by buildship and we'll have to wipe our workspace and start from scratch.
My concern here is not large enough that I think we should block this change since this solves a lot of existing problems and needing to add a setting like the circular dependency (i.e. one that buildship won't pick up itself) one should hopefully be very rare
|
I'm going to go ahead and merge this, as I'm not aware of any troubles it might cause. |
* Remove eclipse conditionals We used to have some meta projects with a `-test` prefix because historically eclipse could not distinguish between test and main source-sets and could only use a single classpath. This is no longer the case for the past few Eclipse versions. This PR adds the necessary configuration to correctly categorize source folders and libraries. With this change eclipse can import projects, and the visibility rules are correct e.x. auto compete doesn't offer classes from test code or `testCompile` dependencies when editing classes in `main`. Unfortunately the cyclic dependency detection in Eclipse doesn't seem to take the difference between test and non test source sets into account, but since we are checking this in Gradle anyhow, it's safe to set to `warning` in the settings. Unfortunately there is no setting to ignore it. This might cause problems when building since Eclipse will probably not know the right order to build things in so more wirk might be necesarry.
* Remove eclipse conditionals We used to have some meta projects with a `-test` prefix because historically eclipse could not distinguish between test and main source-sets and could only use a single classpath. This is no longer the case for the past few Eclipse versions. This PR adds the necessary configuration to correctly categorize source folders and libraries. With this change eclipse can import projects, and the visibility rules are correct e.x. auto compete doesn't offer classes from test code or `testCompile` dependencies when editing classes in `main`. Unfortunately the cyclic dependency detection in Eclipse doesn't seem to take the difference between test and non test source sets into account, but since we are checking this in Gradle anyhow, it's safe to set to `warning` in the settings. Unfortunately there is no setting to ignore it. This might cause problems when building since Eclipse will probably not know the right order to build things in so more wirk might be necesarry.
* Remove eclipse conditionals We used to have some meta projects with a `-test` prefix because historically eclipse could not distinguish between test and main source-sets and could only use a single classpath. This is no longer the case for the past few Eclipse versions. This PR adds the necessary configuration to correctly categorize source folders and libraries. With this change eclipse can import projects, and the visibility rules are correct e.x. auto compete doesn't offer classes from test code or `testCompile` dependencies when editing classes in `main`. Unfortunately the cyclic dependency detection in Eclipse doesn't seem to take the difference between test and non test source sets into account, but since we are checking this in Gradle anyhow, it's safe to set to `warning` in the settings. Unfortunately there is no setting to ignore it. This might cause problems when building since Eclipse will probably not know the right order to build things in so more wirk might be necesarry.
|
Pack-port complete across all active development branches. |
We used to have some meta projects with a
-testprefix becausehistorically eclipse could not distinguish between test and main
source-sets and could only use a single classpath.
This is no longer the case for the past few Eclipse versions.
This PR adds the necessary configuration to correctly categorize source
folders and libraries.
With this change eclipse can import projects, and the visibility rules
are correct e.x. auto compete doesn't offer classes from test code or
testCompiledependencies when editing classes inmain.Unfortunately the cyclic dependency detection in Eclipse doesn't seem to
take the difference between test and non test source sets into account,
but since we are checking this in Gradle anyhow, it's safe to set to
warningin the settings. Unfortunately there is no setting to ignoreit.
This might cause problems when building since Eclipse will probably not
know the right order to build things in so more work might be necessary.
//cc @cbuescher @jpountz @colings86 ( please add other Eclipse users )