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

GRPC pom: Adding operating system specific profiles #42993

Merged
merged 1 commit into from
Sep 4, 2024

Conversation

tmulle
Copy link
Contributor

@tmulle tmulle commented Sep 3, 2024

Fix #42931

In which this PR adds operating system specific profiles to pull only the dependencies needed for the machine being built.
This should help cut down on the pulling of extra files we don't need.

I've run the ./mvnw -Dquickly from the root and all tests passed.
I verified on MacOS that ONLY the mac .exe gets pulled during the test by deleting the ~/.m2/repository/io/grpc/protoc-gen-gprc-java folder prior to running the tests.

I've run the /.mvnw -Dquickly on my Fedora 39 Linux machine and all tests passed.
I verified on Fedora Linux that ONLY the linux-x86_64.exe gets pulled during the test by deleting the ~/.m2/repository/io/grpc/protoc-gen-gprc-java folder prior to running the tests.

I also tried to cut down the file even further by just using the os.detected.classifier in the two protoc dependencies rather than spelling out each arch/os but I got an error from maven saying the version was missing.

I see the os.detected.classifier printed out at the beginning of the tests but it appears it's not available for the grpc tests?

Anyway, this change should work better by eliminating pulling down files for other operatings systems that aren't needed.

I could NOT verify the s390 or ppc archictectures since I don't have those around.. I just had Linux, Mac and windows.

@quarkus-bot quarkus-bot bot added the area/grpc gRPC label Sep 3, 2024
Copy link
Contributor

@melloware melloware left a comment

Choose a reason for hiding this comment

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

Welcome to the club!

Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

Thanks! It looks good to me except I think I would drop the name for macOS and Windows as it doesn't bring anything.

Apart from that, I'm not sure how other unixes should be handled but I suppose we will have to wait for feedback from the field.

Could you adjust and squash? Thanks!

<id>osx-x86_64</id>
<activation>
<os>
<name>Mac OS X</name>
Copy link
Member

Choose a reason for hiding this comment

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

I would drop the name here.

<id>osx-aarch64</id>
<activation>
<os>
<name>Mac OS X</name>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

<id>windows-x86_32</id>
<activation>
<os>
<name>Windows</name>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

<id>windows-x86_64</id>
<activation>
<os>
<name>Windows</name>
Copy link
Member

Choose a reason for hiding this comment

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

Same here.

…dencies that are pulled on each platform

Removing name from os profiles since checking by arch should be enough for now
Copy link
Member

@gsmet gsmet left a comment

Choose a reason for hiding this comment

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

For me, it's all good, thanks!

But I would also like the blessing of @cescoffier .

Copy link

github-actions bot commented Sep 4, 2024

🎊 PR Preview ac5e735 has been successfully built and deployed to https://quarkus-pr-main-42993-preview.surge.sh/version/main/guides/

  • Images of blog posts older than 3 months are not available.
  • Newsletters older than 3 months are not available.

Copy link

quarkus-bot bot commented Sep 4, 2024

Status for workflow Quarkus Documentation CI

This is the status report for running Quarkus Documentation CI on commit 66e1b75.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

Warning

There are other workflow runs running, you probably need to wait for their status before merging.

Copy link

quarkus-bot bot commented Sep 4, 2024

Status for workflow Quarkus CI

This is the status report for running Quarkus CI on commit 66e1b75.

✅ The latest workflow run for the pull request has completed successfully.

It should be safe to merge provided you have a look at the other checks in the summary.

You can consult the Develocity build scans.


Flaky tests - Develocity

⚙️ JVM Tests - JDK 17

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:569)

⚙️ JVM Tests - JDK 21

📦 integration-tests/opentelemetry

io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.testOTelInjections - History

  • Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds. - org.awaitility.core.ConditionTimeoutException
org.awaitility.core.ConditionTimeoutException: Condition with Lambda expression in io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest was not fulfilled within 5 seconds.
	at org.awaitility.core.ConditionAwaiter.await(ConditionAwaiter.java:167)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:78)
	at org.awaitility.core.CallableCondition.await(CallableCondition.java:26)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:1006)
	at org.awaitility.core.ConditionFactory.until(ConditionFactory.java:975)
	at io.quarkus.it.opentelemetry.OpenTelemetryInjectionsTest.reset(OpenTelemetryInjectionsTest.java:26)
	at java.base/java.lang.reflect.Method.invoke(Method.java:580)

@gsmet gsmet merged commit 80190e8 into quarkusio:main Sep 4, 2024
38 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.16 - main milestone Sep 4, 2024
@gsmet
Copy link
Member

gsmet commented Sep 4, 2024

Nice improvement, thanks!

@gsmet
Copy link
Member

gsmet commented Sep 5, 2024

When we did that, I thought "will it work with Gradle?" and I was pleasantly surprised.

But actually it doesn't, see #43038 for all the gory details and the revert.

@gsmet gsmet removed this from the 3.16 - main milestone Sep 5, 2024
@melloware
Copy link
Contributor

Yuck. I guess I will keep my Gradle rant to myself...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

GRPC example on Fedora 39 doesn't compile
4 participants