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

Quarkiverse extensions can build on other OSes too #23889

Merged
merged 1 commit into from Mar 4, 2022

Conversation

gastaldi
Copy link
Contributor

@gastaldi gastaldi commented Feb 22, 2022

This PR introduces some improvements to Quarkiverse extensions' build script:

  • Changes the build script to easily allow building on other OSes (commented out by default)
  • Use actions/setup-java's cache

@quarkus-bot quarkus-bot bot added area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform labels Feb 22, 2022
@quarkus-bot
Copy link

quarkus-bot bot commented Feb 23, 2022

This workflow status is outdated as a new workflow run has been triggered.

Failing Jobs - Building 4096d94

Status Name Step Failures Logs Raw logs
✔️ JVM Tests - JDK 11
JVM Tests - JDK 11 Windows Build Failures Logs Raw logs
✔️ JVM Tests - JDK 17

Full information is available in the Build summary check run.

Failures

⚙️ JVM Tests - JDK 11 Windows #

- Failing: extensions/vertx/deployment 
! Skipped: core/test-extension/deployment extensions/agroal/deployment extensions/amazon-lambda-http/deployment and 315 more

📦 extensions/vertx/deployment

io.quarkus.vertx.DuplicatedContextTest.testThatBlockingEventConsumersAreCalledOnDuplicatedContext - More details - Source on GitHub

(RECIPIENT_FAILURE,8185) io.smallrye.mutiny.TimeoutException
	at io.vertx.core.eventbus.Message.fail(Message.java:141)
	at io.quarkus.vertx.runtime.VertxRecorder$3$1$1.handle(VertxRecorder.java:113)

Copy link
Contributor

@maxandersen maxandersen left a comment

Choose a reason for hiding this comment

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

Assuming tests passes lgtm

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.

Just want to open a debate, please dismiss if you think it's useless.

Is it really a good thing to multiply our CI usage by 3, when in most cases this will have little value? I would think it could be enabled by the Quarkiverse authors when they think it makes sense for their libraries (we had one so far AFAICS).

Also, we wanted to enable native testing by default for Quarkiverse extensions and it will require some non trivial work with Windows in the matrix.
So if we decide to do that, I think we should add native testing on Linux (and I would limit it to Linux for the default behavior) because it might not be easy for the Quarkiverse contributor to do it afterwards with this new setup.

@gsmet
Copy link
Member

gsmet commented Feb 23, 2022

Btw this question: Is it really a good thing to multiply our CI usage by 3, when in most cases this will have little value? has 3 dimensions:

  • GitHub Actions resources in general
  • The planet
  • Our GitHub Actions limited resources -> we multiply by 3 our GitHub Actions usage and I know some of you were already worried about the day when we would hit the limit

@gsmet
Copy link
Member

gsmet commented Feb 23, 2022

I wouldn't be against having this matrix setup with Linux by default and a commented line with Linux/Windows.

I see little value in testing with macOS in the Java world, except in very specific cases.

@maxandersen
Copy link
Contributor

You are right @gsmet it does not make sense to enable This for All by default.

Maybe parameteriZe the matrix and let users enable whats needed.

Osx is relevant for native but yes; not by default.

@gastaldi
Copy link
Contributor Author

@gsmet The reason why I am doing this is because some Quarkiverse builds were reported to fail in Windows (see quarkiverse/quarkus-openapi-generator#7).

But I agree that it wouldn't be necessary from day one. I like your suggestion of having the matrix setup with Linux by default and some commented out lines for Windows. I also agree that testing in macOS is not very valuable (perhaps if one likes to investigate too many open files errors, but that's another story) :)

@gsmet
Copy link
Member

gsmet commented Feb 23, 2022

some Quarkiverse builds were reported to fail in Windows (see quarkiverse/quarkus-openapi-generator#7).

Yes, I have seen that, that's the one occurrence I was referencing to. I'm just unsure we should generalize this based on a few occurrences where it makes absolutely sense.

CI has a cost, even if it appears free to us, and I think we should try to be good citizens about it when we can.

Use actions/setup-java's cache
@gastaldi gastaldi changed the title Quarkiverse build on Windows Quarkiverse extensions can build on other OSes too Feb 23, 2022
@gastaldi gastaldi requested a review from gsmet February 23, 2022 18:36
@gsmet gsmet merged commit 3aca34d into quarkusio:main Mar 4, 2022
@quarkus-bot quarkus-bot bot added this to the 2.8 - main milestone Mar 4, 2022
@gastaldi gastaldi deleted the quarkiverse_build branch March 4, 2022 14:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/devtools Issues/PR related to maven, gradle, platform and cli tooling/plugins area/platform Issues related to definition and interaction with Quarkus Platform
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants