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

Skip CDS test if the JDK cannot create AppCDS #37679

Merged
merged 1 commit into from
Dec 12, 2023
Merged

Conversation

dmlloyd
Copy link
Member

@dmlloyd dmlloyd commented Dec 12, 2023

This assumes that we are only testing the CDS works if it can be created. If it cannot be created because the JVM does not support it, we should not fail the entire build.

/cc @geoand just to make sure this makes sense.

This assumes that we are only testing the CDS works if it can be created. If it cannot be created because the JVM does not support it, we should not fail the entire build.
Copy link

quarkus-bot bot commented Dec 12, 2023

✔️ 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.

@geoand
Copy link
Contributor

geoand commented Dec 12, 2023

I'm interested in hearing about which JVM doesn't support AppCDS 😎

@geoand geoand merged commit 419ed1a into quarkusio:main Dec 12, 2023
17 checks passed
@quarkus-bot quarkus-bot bot added this to the 3.7 - main milestone Dec 12, 2023
@dmlloyd dmlloyd deleted the cds branch December 12, 2023 12:35
@dmlloyd
Copy link
Member Author

dmlloyd commented Dec 12, 2023

Mine, apparently! 🤷‍♂️

@geoand
Copy link
Contributor

geoand commented Dec 12, 2023

Fair enough :)

@gsmet
Copy link
Member

gsmet commented Dec 12, 2023

And we are sure we will NEVER have this message when for some reason something is failing?

The message looks very broad.

@dmlloyd
Copy link
Member Author

dmlloyd commented Dec 12, 2023

It's tricky... AppCDS is definitely not a part of the platform spec. So, it doesn't make sense to fail the build unless we want to make it a requirement.

That said, how can we detect the possible reasons for failing when there isn't even a specification for it? In fact Quarkus doesn't even fail when AppCDS generation fails, it just prints a warning (the one that the code in the PR looks for).

I know how to make the check even less strict (change it to assume that the log contains e.g. AppCDS successfully created at). However I don't know of any way to make it more strict (in other words, to detect specific kinds of failures and trigger the build failure based on that). I'm open to ideas and would gladly produce a follow-up PR to improve this if anyone thinks of anything. I'll also poke around the AppCDS code to see if there's something we can use.

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