Skip to content

Conversation

@zakkak
Copy link
Collaborator

@zakkak zakkak commented Oct 26, 2021

No description provided.

@zakkak zakkak requested a review from fniephaus October 26, 2021 19:30
@zakkak zakkak self-assigned this Oct 26, 2021
@fniephaus
Copy link
Member

Why run with libgraal? Any particular reason?

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 26, 2021

Why run with libgraal? Any particular reason?

AFAIK, upstream releases are using libgraal by default. As seen in graalvm#304 (comment) libgraal has a significant impact on the performance of native-image so I believe that it would be best to use a build that is as close as possible to the future GraalVM release.

@fniephaus
Copy link
Member

Building libgraal probably takes longer than it saves time by improving performance on CI.

Why not use the pre-installed GraalVM installation at $GRAALVM_11_ROOT instead? GitHub automatically pulls the latest GraalVM release.

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 26, 2021

Building libgraal probably takes longer than it saves time by improving performance on CI.

This is not to improve the performance on the CI, but to make sure that we are testing with a configuration as close as possible to the one that will be released. Note also that the additional time spent to build libgraal is not that significant.

This PR took ~6.5m to build native-image, while before it used to take ~4.5m

Furthermore we are only building GraalVM once for all the integration tests, so that is ~2m extra CI time per day.

Why not use the pre-installed GraalVM installation at $GRAALVM_11_ROOT instead? GitHub automatically pulls the latest GraalVM release.

The aim of this pipeline is to test the tip of oracle/graal with the tip of quarkusio/quarkus. Testing with the latest release is not enough to ensure that Quarkus will be ready for the next release on time. Note however that we already perform similar tests in graalvm/mandrel so if that pipeline appears of no use to the GraalVM team we could consider removing it.

@fniephaus
Copy link
Member

fniephaus commented Oct 26, 2021

Thanks, just wanted to understand why you're proposing this change. +2min sounds more than reasonable. :)

@zakkak
Copy link
Collaborator Author

zakkak commented Oct 27, 2021

Thanks, just wanted to understand why you're proposing this change. +2min sounds more than reasonable. :)

Sure, I should have added some info in the description of the PR in the first place. Thanks for reviewing this!

For the record, the tests are currently failing due to #3924, Quarkus' PR quarkusio/quarkus#20968 fixes the issue.

@fniephaus fniephaus self-assigned this Oct 28, 2021
@zakkak zakkak deleted the fix-quarkus-ci branch October 28, 2021 11:44
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.

3 participants