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.

Sign up for GitHub

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

Jump to bottom

Add Java8 support #353

Merged
merged 16 commits into from
Jun 26, 2024
Merged

Add Java8 support #353

merged 16 commits into from
Jun 26, 2024

Conversation

psx95
Copy link
Contributor

@psx95 psx95 commented Jun 22, 2024 •
edited
Loading

Description

This PR makes the minimum supported version for artifacts generated from this repository to Java 8 by default.
Operational modules like e2e-testing, may use higher Java versions due to dependency constraints.

Testing

  • ./gradlew clean build successfully runs
  • Looking at the class files present in generated module JARs using javap -v path/to/classfile/in/jar reveals major version as 52 (Java 8) and 55 (Java 11) for expected modules.
  • Using APIs not available in Java 8 for modules configured to produce Java 8 bytecode result in build error. Note that since test code is not part of the final JAR, tests may still use APIs upto Java 11.

psx95
psx95 commented Jun 22, 2024
View reviewed changes
build.gradle
@@ -234,12 +240,6 @@ subprojects {
archives javadocJar, sourcesJar
}

javadoc {
Copy link
Contributor Author

@psx95 psx95 Jun 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

Minimum JDK version required for building this project is JDK 11, html5 is default and is no longer required.

@psx95
fix typo
4d14928
@psx95
Update docker image to include JDK 8
cf72700 
The docker image used in e2e tests requires JDK8 to compile and build
trace exporter and gcp propagators. JDK8 is explicitly required because
the builds of these artifacts are configured using toolchains.
@psx95
Set language version in root project to Java 11
725c4a1 
Setting the language version in root project to 11 avoids the need of
having multiple JDKs to build all artifacts in this project.

Java 8 bytecode compatibility is ensured using --release flag in Java
compiler available starting Java 9.
@psx95
Additional JDK 8 installaion no longer required
a8019b3
@psx95
Add comment explaining need for Java 11
c04a59f
@psx95
Revert changes to make tests compatible with Java 8
cfa27e5
@psx95
Java 11 not required to Run this example
42f82bc 
Java 11 specific APIs were only used in tests.
@psx95
Update README to indicate Java version compatibility
7669406
@psx95 psx95 marked this pull request as ready for review June 24, 2024 21:58
@psx95 psx95 requested a review from a team as a code owner June 24, 2024 21:58
@psx95 psx95 requested review from punya, dashpole and aabmass June 24, 2024 21:58
aabmass
aabmass approved these changes Jun 24, 2024
View reviewed changes
Copy link
Contributor

@aabmass aabmass left a comment

Choose a reason for hiding this comment

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

@punya any concerns about not actually testing against Java8? @psx95 mentioned that we weren't doing it before either so this seems reasonable. But I'm no Java expert and don't know what might go wrong

build.gradle Outdated Show resolved Hide resolved
e2e-test-server/build.gradle Outdated Show resolved Hide resolved
exporters/trace/src/main/java/com/google/cloud/opentelemetry/trace/InternalTraceExporter.java Outdated Show resolved Hide resolved
@psx95
Copy link
Contributor Author

psx95 commented Jun 25, 2024 •
edited
Loading

@punya any concerns about not actually testing against Java8? @psx95 mentioned that we weren't doing it before either so this seems reasonable. But I'm no Java expert and don't know what might go wrong

I think the ideal way going forward would be to write a CI that loads the modules/JAR intended to be distributed as Java8 compatible on a VM that only has Java 8 installed on it.

I don't think we should force the current unit tests to use Java8 API.

What might go wrong?

A bump in gradle version or some gradle config changes could cause the --release flag to be ignored, resulting in artifacts that are not Java 8 compatible.

Edit: This would not have been a concern if were using two different JVM toolchains (Java 8 for all projects and Java 11 for e2e-tests). I had previously considered this way, but changed the implementation due to the following reasons:

  • Building this project would require the presence of two JDKs on the user's machine.
  • Using multiple toolchains seems inefficient and would increase the CloudBuild time since we would need to download and install two versions of Java in all docker images.
  • Additionally, docker images would now have to be custom, since I could not find an official docker image for gradle + Java 8 + Java 11.

punya
punya approved these changes Jun 26, 2024
View reviewed changes
Copy link
Contributor

@punya punya left a comment

Choose a reason for hiding this comment

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

We're lucky that we hardly took any dependencies on new syntax or libraries in all this time!

@psx95
Copy link
Contributor Author

psx95 commented Jun 26, 2024 •
edited
Loading

We're lucky that we hardly took any dependencies on new syntax or libraries in all this time!

Yes! Most of the new syntax is used only in unit tests - cfa27e5.

@psx95 psx95 merged commit 44da9c4 into GoogleCloudPlatform:main Jun 26, 2024
18 checks passed
@psx95 psx95 deleted the java8-support branch June 26, 2024 18:25
zhumin8 referenced this pull request in googleapis/sdk-platform-java Jul 9, 2024
@renovate-bot
deps: update dependency com.google.cloud.opentelemetry:detector-resou…
b3ec93f 
…rces-support to v0.30.0 (#2975)

[![Mend
Renovate](https://app.renovatebot.com/images/banner.svg)](https://renovatebot.com)

This PR contains the following updates:

| Package | Change | Age | Adoption | Passing | Confidence |
|---|---|---|---|---|---|
|
[com.google.cloud.opentelemetry:detector-resources-support](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java)
| `0.29.0` -> `0.30.0` |
[![age](https://developer.mend.io/api/mc/badges/age/maven/com.google.cloud.opentelemetry:detector-resources-support/0.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![adoption](https://developer.mend.io/api/mc/badges/adoption/maven/com.google.cloud.opentelemetry:detector-resources-support/0.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![passing](https://developer.mend.io/api/mc/badges/compatibility/maven/com.google.cloud.opentelemetry:detector-resources-support/0.29.0/0.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|
[![confidence](https://developer.mend.io/api/mc/badges/confidence/maven/com.google.cloud.opentelemetry:detector-resources-support/0.29.0/0.30.0?slim=true)](https://docs.renovatebot.com/merge-confidence/)
|

---

> [!WARNING]
> Some dependencies could not be looked up. Check the Dependency
Dashboard for more information.

---

### Release Notes

<details>
<summary>GoogleCloudPlatform/opentelemetry-operations-java
(com.google.cloud.opentelemetry:detector-resources-support)</summary>

###
[`v0.30.0`](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/releases/tag/v0.30.0)

[Compare
Source](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/compare/v0.29.0...v0.30.0)

#### Release Highlights

- Minimum supported Java version set to **Java 8** for all consumable
artifacts.

#### What's Changed

- Udpate maven-badges to link to sonatype central by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/349](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/349)
- Make the trace flags optional when parsing x-cloud-trace-context by
[@&#8203;sawadakaku](https://togithub.com/sawadakaku) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/350](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/350)
- Update Metrics Exporter README by
[@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/352](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/352)
- Add Java8 support by [@&#8203;psx95](https://togithub.com/psx95) in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/353](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/353)

#### New Contributors

- [@&#8203;sawadakaku](https://togithub.com/sawadakaku) made their first
contribution in
[https://github.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/350](https://togithub.com/GoogleCloudPlatform/opentelemetry-operations-java/pull/350)

**Full Changelog**:
GoogleCloudPlatform/opentelemetry-operations-java@v0.29.0...v0.30.0

</details>

---

### Configuration

📅 **Schedule**: Branch creation - At any time (no schedule defined),
Automerge - At any time (no schedule defined).

🚦 **Automerge**: Disabled by config. Please merge this manually once you
are satisfied.

♻ **Rebasing**: Whenever PR becomes conflicted, or you tick the
rebase/retry checkbox.

🔕 **Ignore**: Close this PR and you won't be reminded about this update
again.

---

- [ ] <!-- rebase-check -->If you want to rebase/retry this PR, check
this box

---

This PR has been generated by [Mend
Renovate](https://www.mend.io/free-developer-tools/renovate/). View
repository job log
[here](https://developer.mend.io/github/googleapis/sdk-platform-java).

<!--renovate-debug:eyJjcmVhdGVkSW5WZXIiOiIzNy40MjAuMSIsInVwZGF0ZWRJblZlciI6IjM3LjQyMC4xIiwidGFyZ2V0QnJhbmNoIjoibWFpbiIsImxhYmVscyI6W119-->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@aabmass aabmass aabmass approved these changes

@punya punya punya approved these changes

@dashpole dashpole Awaiting requested review from dashpole

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@psx95 @punya @aabmass