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

Add itext core metadata and tests #470

Merged
merged 2 commits into from
Apr 16, 2024
Merged

Conversation

vitali-pr
Copy link
Contributor

What does this PR do?

Add itext core metadata and tests

Code sections where the PR accesses files, network, docker or some external service

Checklist before merging

@vitali-pr vitali-pr requested a review from a team as a code owner March 26, 2024 05:47
@vitali-pr vitali-pr requested a review from olpaw March 26, 2024 05:47
@vitali-pr vitali-pr force-pushed the itextcore-configs branch 4 times, most recently from 3b8e332 to 56d623f Compare April 12, 2024 09:49
@vitali-pr
Copy link
Contributor Author

Hi @olpaw ,
Somewhere in another PR I saw that it's ok to ping you if the PR is not processed for quite some time.
It's been a while since this PR created. Is there a chance you have a look at it? Thanks!

@olpaw
Copy link
Member

olpaw commented Apr 15, 2024

Hi @vitali-pr

Thanks for the reminder. This got lost in the sea of emails. I will have a look this week.

@olpaw olpaw self-assigned this Apr 15, 2024
@@ -0,0 +1,11 @@
[
{
"latest": true,
Copy link
Member

@olpaw olpaw Apr 15, 2024

Choose a reason for hiding this comment

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

Use either

"latest": true,

or

"default-for": "8\\.0\\..*"

Using them both makes the "default-for" irrelevant thus potentially confusing.

Same for other index.json files in your PR.

See https://github.com/oracle/graalvm-reachability-metadata/blob/master/CONTRIBUTING.md

Since 8.x.y is really the latest and you do not have metadata for older versions I suggest you just remove all the "default-for" ... entries in your index.json files.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Pushed the required changes amended with the initial commit.

Copy link
Member

@olpaw olpaw left a comment

Choose a reason for hiding this comment

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

This PR looks clean. Only thing that needs to be changed before merging is to remove the unnecessary use of "default-for": ... as already mentioned in #470 (comment)

@vitali-pr vitali-pr requested a review from olpaw April 15, 2024 13:46
@vitali-pr
Copy link
Contributor Author

vitali-pr commented Apr 15, 2024

@olpaw , I see the failed checks. I didn't add conditions for those io resources intentionally. This is how we use them on java. They are coming with a (separate) font-asian jar. If it's in class path, they are available for a user to load.

@olpaw
Copy link
Member

olpaw commented Apr 15, 2024

This is how we use them on java. They are coming with a (separate) font-asian jar. If it's in class path, they are available for a user to load.

So there are no class files in font-asian jar that you could use as typeReachable?

Even if so ... it should be fine to use

      "typeReachable": "com.itextpdf.io.font.Type1Font"

even for the resources in font-asian jar, because that jar file only makes sense in combination with the com.itextpdf.io main jar. Thus it's save to assume com.itextpdf.io.font.Type1Font is there, if we have font-asian jar on the classpath.

@olpaw
Copy link
Member

olpaw commented Apr 15, 2024

Thus it's save to assume com.itextpdf.io.font.Type1Font is there, if we have font-asian jar on the classpath.

But it would be even better if you know of a class that only gets reachable if one of the resources from font-asian.jar are used. This way an image using com.itextpdf.io would be smaller if none of the resources from font-asian.jar are used by the application that uses com.itextpdf.io.

@vitali-pr
Copy link
Contributor Author

vitali-pr commented Apr 16, 2024

Thus it's save to assume com.itextpdf.io.font.Type1Font is there

@olpaw , You are right, it will be com.itextpdf.io.font.CidFont then. But the problem is that we don't test these resources using CidFont class. So without a fake CidFont usage in the tests, the tests will just fail in native. Also who knows how users are supposed to use these resources, maybe they'd implement their own FontProgram.
Anyway, I think I found a compromise by just adding com.itextpdf.io.font.CjkResourceLoader type as a condition. I've added a new commit to the review.

But it would be even better if you know of a class that only gets reachable if one of the resources from font-asian.jar are used.

There are no such per cmap classes.

@olpaw olpaw merged commit dcd15c1 into oracle:master Apr 16, 2024
30 checks passed
@olpaw
Copy link
Member

olpaw commented Apr 16, 2024

@vitali-pr your PR is now on master. Thank you for your contribution!

@vitali-pr vitali-pr deleted the itextcore-configs branch April 16, 2024 09:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants