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

Exclude Netty's reflection configuration files #29493

Merged
merged 1 commit into from
Nov 30, 2022

Conversation

zakkak
Copy link
Contributor

@zakkak zakkak commented Nov 25, 2022

These are causing warnigns during native image compilation as they try to access classes that are not on the
classpath (e.g. net.jpountz.lz4.LZ4Exception).

Closes #29413

@zakkak zakkak requested review from Sanne and geoand November 25, 2022 14:21
@zakkak
Copy link
Contributor Author

zakkak commented Nov 25, 2022

@quarkus-bot

This comment has been minimized.

@zakkak
Copy link
Contributor Author

zakkak commented Nov 25, 2022

Windows run failed with:

The system cannot find the path specified.

I guess I am hitting what Sanne described, I will create two separate build items instead of a single one with a complex regex.

@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch 2 times, most recently from d9f31bf to 1916593 Compare November 25, 2022 20:40
@zakkak
Copy link
Contributor Author

zakkak commented Nov 25, 2022

@quarkus-bot

This comment has been minimized.

@zakkak
Copy link
Contributor Author

zakkak commented Nov 26, 2022

Windows still failing, this time with:

Error: Unknown argument: quarkus-integration-test-resteasy-jackson-999-SNAPSHOT-runner

I will come back to this next week

@zakkak zakkak marked this pull request as draft November 26, 2022 06:57
@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from 1916593 to 084045c Compare November 28, 2022 19:52
@zakkak zakkak marked this pull request as ready for review November 28, 2022 22:14
@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from 084045c to bd6c0a0 Compare November 28, 2022 22:14
@zakkak
Copy link
Contributor Author

zakkak commented Nov 28, 2022

It looks like windows don't like .* in the exclude-config patterns.

@Sanne I changed the Oracle-JDBC pattern as well since it was also not working on windows. Please have a look.

It looks like an upstream issue to me so I will try to create a simple reproducer not relying on Quarkus and see where it takes me.

@zakkak zakkak requested a review from geoand November 28, 2022 22:17
@Sanne
Copy link
Member

Sanne commented Nov 29, 2022

@Sanne I changed the Oracle-JDBC pattern as well since it was also not working on windows. Please have a look.

are you sure of that? It was passing the tests AFAIK - or was this being disabled in some way?


public class NettyOverrideMetadata {

static final String NETTY_CODEC_JAR_MATCH_REGEX = "io\\.netty\\.netty-codec";
Copy link
Member

Choose a reason for hiding this comment

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

I'm confused as to why you don't need an ending * - is it also being treated as matching a prefix? Might need to clarify the documentation about that.

Copy link
Contributor Author

@zakkak zakkak Nov 29, 2022

Choose a reason for hiding this comment

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

Internally native-image uses Pattern.compile(excludeJar).matcher(path).find() which matches even without the * in the beginning or the end.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think we can also just quote the regex though. I will be able to test it in the next couple of hours.

@zakkak
Copy link
Contributor Author

zakkak commented Nov 29, 2022

@Sanne I changed the Oracle-JDBC pattern as well since it was also not working on windows. Please have a look.

are you sure of that? It was passing the tests AFAIK - or was this being disabled in some way?

The only native test being run on windows appears to be resteasy-jackson

{
"category": "Windows - RESTEasy Jackson",
"timeout": 20,
"test-modules": "resteasy-jackson",
"os-name": "windows-latest"
},

and I think it has been that way since day one.

Copy link
Member

@Sanne Sanne left a comment

Choose a reason for hiding this comment

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

@zakkak many thanks for clarifying - I remember that before I separated the two exclusions it did fail on Windows, but I guess that was a compilation failure and not a test failure. Nice catch, thanks.

But in this case.. would you mind separating the two fixes? The oracle JDBC one might be important to backport, and even if we don't it would be nice to have it called out in the changelogs.

@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from bd6c0a0 to bf25791 Compare November 29, 2022 11:41
@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from bf25791 to 2f7386d Compare November 29, 2022 11:43
@zakkak
Copy link
Contributor Author

zakkak commented Nov 29, 2022

But in this case.. would you mind separating the two fixes? The oracle JDBC one might be important to backport, and even if we don't it would be nice to have it called out in the changelogs.

Done in #29552

I also updated this PR to only include the Netty fix, but it depends on #29552 so I am marking it as draft (again).

@zakkak zakkak marked this pull request as draft November 29, 2022 11:43
@quarkus-bot

This comment has been minimized.

@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from 2f7386d to 8018837 Compare November 29, 2022 16:28
These are causing warnings during native image compilation as they try
to access classes that are not on the
classpath (e.g. `net.jpountz.lz4.LZ4Exception`).

Closes quarkusio#29413
@zakkak zakkak force-pushed the 2022-11-25-fix-29413 branch from 8018837 to 79bd91f Compare November 29, 2022 16:28
@zakkak
Copy link
Contributor Author

zakkak commented Nov 29, 2022

As mentioned in #29552 (comment)

This turns out to be more complex than I thought. The \" works fine on windows but not on linux.
As a result, for a quick-fix I am switching to my previous approach that does not rely on the quotes at the cost of using more expressions than one and avoiding the use of .*.

@zakkak zakkak marked this pull request as ready for review November 29, 2022 20:18
@quarkus-bot

This comment has been minimized.

@gsmet
Copy link
Member

gsmet commented Feb 22, 2023

Adding for backport to 2.13 because of #30508

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.

Recent change introduced warnings while building native image of MongoDB Client (and possibly others)
4 participants