The review phase is an important part of contributing to Elasticsearch, but we
do not mention it in our instructions to contributors. This commit adds some
notes on how contributions are reviewed and describes some common reasons for
rejection.
Co-authored-by: Colin Goodheart-Smithe <colings86@users.noreply.github.com>
* Remove usage of deprecated testCompile configuration
* Replace testCompile usage by testImplementation
* Make testImplementation non transitive by default (as we did for testCompile)
* Update CONTRIBUTING about using testImplementation for test dependencies
* Fail on testCompile configuration usage
This commit removes the configuration time vs execution time distinction
with regards to certain BuildParms properties. Because of the cost of
determining Java versions for configuration JDK locations we deferred
this until execution time. This had two main downsides. First, we had
to implement all this build logic in tasks, which required a bunch of
additional plumbing and complexity. Second, because some information
wasn't known during configuration time, we had to nest any build logic
that depended on this in awkward callbacks.
We now defer to the JavaInstallationRegistry recently added in Gradle.
This utility uses a much more efficient method for probing Java
installations vs our jrunscript implementation. This, combined with some
optimizations to avoid probing the current JVM as well as deferring
some evaluation via Providers when probing installations for BWC builds
we can maintain effectively the same configuration time performance
while removing a bunch of complexity and runtime cost (snapshotting
inputs for the GenerateGlobalBuildInfoTask was very expensive). The end
result should be a much more responsive build execution in almost all
scenarios.
(cherry picked from commit ecdbd37f2e0f0447ed574b306adb64c19adc3ce1)
This commit makes a number of improvements when importing the
Elasticsearch project into IntelliJ IDEA. Specifically:
- Contributing documentation has been updated to reflect that the
'idea' task should no long be used and Gradle project import is
instead the officially supported way of setting up the project.
- Attempts to run the 'idea' task will result in a failure with a
message directing folks to our CONTRIBUTING.md document.
- The project JDK is explicit set rather that using whatever JAVA_HOME
is.
- Gradle build operation delegation is disabled, and test execution is
configured to 'choose per test'.
- Gradle is configured to inherit the project JDK.
- Some code style conventions are automatically configured.
- File encoding is explicitly set to UTF-8.
- Parallel module compilation is enabled and deprecated feature
warnings are disabled.
- A remote debug run configuration using listen mode is created.
- JUnit runner is configured with required system properties.
- License headers are configured such that Apache 2 is the default
notice added to all source files with exception of source in /x-pack
which will use the Elastic license.
We have a long history of advancing the required compiler to the newest
JDK. JDK 13 has been with us for awhile, but we were blocked from
upgrading since Gradle was not compatible with JDK 13. With the
advancement in our project to Gradle 6 which supports JDK 13, we can now
advance our minimum compiler version. This commit updates the minimum
compiler version to JDK 13.
Some unit test checking locale sensitive functionality require the
-Djava.locale.providers=SPI,COMPAT flag to be set. When running tests though
gradle we pass this already to the BuildPlugin, but running from the IDE this
might need to be set manually. Adding a note explaining this to the
CONTRIBUTING.md doc and leaving a note in the test comment of
SearchQueryIT.testRangeQueryWithLocaleMapping which is a test we know
that suffers from this issue.
With this commit we add a clarifying note in the contribution guidelines
that our examples show the usage on Unix and also explain how to invoke
the Gradle wrapper script on Windows.
Closes#49521
The team sometimes get questions around the use of `!foo` vs. `foo == false` in
PRs and reviews (e.g. #48615). This change adds a bullet point to CONTRIBUTING.md
to make expectations here clearer and gives us something to point to in case of
discussion.
Backport of #48446. Update the Eclipse formatter settings as follows:
* Split arguments for methods, constructors etc into separate lines when
they wrap
* Enable "joined wrapped lines", which basically means the formatter
will ignore existing line breaks and join and re-wrap statements. I
had originally hesitated about enabling this, but without it the
codebase's style won't be as consistent as it should be, defeating the
point of all this formatting business.
* Disable formatting between `// tag::` and `// end::` comments, which
are the code snippet markers for the docs. They need a different line
length, which isn't possible to configure separately.
* Detect and preserve aligned line comments at the end of consecutive
lines.
* And a bunch of other tweaks.
Also update the contribution guidelines.
Today our instructions in `CONTRIBUTING.md` regarding license headers are a
little unclear. This commit clarifies the exact headers that are required for
contributions.
* 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.
This commit adds a Java source formatter and checker into the build process.
This is not yet enabled for any sub-projects - to format and check a
sub-project, add its Gradle path into `build.gradle` and run:
./gradlew spotlessApply
to format, and:
./gradlew spotlessJavaCheck
# or:
./gradlew precommit
to verify formatting.
After we bumped the JDK compiler minimum requirements to JDK 11, we
missed updating the contributing docs to point to JDK 11 instead of JDK
10. This commit fixes that.
Additionally, when we stopped requiring JDK 7 to run the reindex from
old tests, we did not update the contributing docs to reflect this. This
commit also fixes that.
This commit removes the suggestions to send a PR with instructions for
more IDEs on disabling wildcard imports. This is not explicitly needed
both because the main IDEs instructions already exist, and any part of
the docs are welcome for PRs to add more clarity.
closes#34415
This reworks how we configure the `shadow` plugin in the build. The major
change is that we no longer bundle dependencies in the `compile` configuration,
instead we bundle dependencies in the new `bundle` configuration. This feels
more right because it is a little more "opt in" rather than "opt out" and the
name of the `bundle` configuration is a little more obvious.
As an neat side effect of this, the `runtimeElements` configuration used when
one project depends on another now contains exactly the dependencies needed
to run the project so you no longer need to reference projects that use the
shadow plugin like this:
```
testCompile project(path: ':client:rest-high-level', configuration: 'shadow')
```
You can instead use the much more normal:
```
testCompile "org.elasticsearch.client:elasticsearch-rest-high-level-client:${version}"
```
This bundles the x-pack:protocol project into the x-pack:plugin:core
project because we'd like folks to consider it an implementation detail
of our build rather than a separate artifact to be managed and depended
on. It is now bundled into both x-pack:plugin:core and
client:rest-high-level. To make this work I had to fix a few things.
Firstly, I had to make PluginBuildPlugin work with the shadow plugin.
In that case we have to bundle only the `shadow` dependencies and the
shadow jar.
Secondly, every reference to x-pack:plugin:core has to use the `shadow`
configuration. Without that the reference is missing all of the
un-shadowed dependencies. I tried to make it so that applying the shadow
plugin automatically redefines the `default` configuration to mirror the
`shadow` configuration which would allow us to use bare project references
to the x-pack:plugin:core project but I couldn't make it work. It'd *look*
like it works but then fail for transitive dependencies anyway. I think
it is still a good thing to do but I don't have the willpower to do it
now.
Finally, I had to fix an issue where Eclipse and IntelliJ didn't properly
reference shadowed transitive dependencies. Neither IDE supports shadowing
natively so they have to reference the shadowed projects. We fix this by
detecting `shadow` dependencies when in "Intellij mode" or "Eclipse mode"
and adding `runtime` dependencies to the same target. This convinces
IntelliJ and Eclipse to play nice.
Adds a description of the most important subdirectories to
`CONTRIBUTING.md` to help folks that are less familiar with the project
get their bearings. It reflects that state of the project right now so
it will inevitably go out of date. But I'll try and keep it up to date.
* add copyright/scope configuration for intellij
This commit introduces a section discussing the ability to define
copyright rules in intellij for inserting the appropriate copyright
headers into files across both apache2-Elasticsearch, as well as the
commercial code under `x-pack`.
Some other re-organization was made to create more sub-structure
* update to reflect reviewer comments
The "Shorten Command Line" setting in Intellij's JUnit test configuration
was recommended to change to `classpath file`. This setting has been
causing issues with JDK9 where some modules were not being found at
runtime. This PR removes the recommendation to change this setting and
instead asks that users verify that it is set to `user-local default:none`.
This commit modifies the build to require JDK 9 for
compilation. Henceforth, we will compile with a JDK 9 compiler targeting
JDK 8 as the class file format. Optionally, RUNTIME_JAVA_HOME can be set
as the runtime JDK used for running tests. To enable this change, we
separate the meaning of the compiler Java home versus the runtime Java
home. If the runtime Java home is not set (via RUNTIME_JAVA_HOME) then
we fallback to using JAVA_HOME as the runtime Java home. This enables:
- developers only have to set one Java home (JAVA_HOME)
- developers can set an optional Java home (RUNTIME_JAVA_HOME) to test
on the minimum supported runtime
- we can test compiling with JDK 9 running on JDK 8 and compiling with
JDK 9 running on JDK 9 in CI
We have agreed to introduce the Gradle wrapper to simplify workflows for
developers, and managing infrastructure (e.g., CI, release builds, etc.)
as well as consideration for the fact that other projects in our stack
use Gradle and do not necessarily want to be tied to our Gradle version.
Relates #28065
* Improved paragraph describing how to run unit tests from Intellij.
* Added information about how to run a local copy of elasticsearch from the source.
Sarat Vemulapalli
Dawn Foster
Tyler Ohlsen
CEHENKLE
Harold Wang
Adam Locke
Ayush Eshan
James Rodewig
Rene Groeschke
David Turner
Nik Everett
Mark Vieira
Jason Tedor
Christoph Büscher
Daniel Mitterdorfer
Rory Hunter
Alpar Torok
CJ Cenizal
Heather McCartney
王振荟
Ryan Ernst
Kazuhiro Sera
aaronoah
Lisa Cawley
Tal Levy
Mikal
hyeonyoung na
Doug Turnbull
Avneesh Chadha