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

fix DOM2DTM to work in J9 #1760

Closed
wants to merge 1 commit into from
Closed

Conversation

Taywee
Copy link
Contributor

@Taywee Taywee commented May 15, 2018

J9 wasn't coping with hot-patching standard library packages with a user class. This was done to work around a package private constructor. This is fixed by moving into a local package and copying the class with the package private constructor into a local copy.

Note that DOM2DTMdefaultNamespaceDeclarationNode in copied almost entirely ver-batim out of the OpenJDK source tree. The only changes are the package and the import of the DTMException, which now uses the external version instead of the internal one.

closes #1759

J9 wasn't coping with hot-patching standard library packages with a user
class.  This was done to work around a package private constructor.
This is fixed by moving into a local package and copying the class with
the package private constructor into a local copy.
@flavorjones
Copy link
Member

Thank you for submitting this PR!

@jvshahid and @kares I'd like your approval before merging. I'm also curious if we're OK with using OpenJDK code from a license perspective?

@kares
Copy link
Contributor

kares commented Jun 16, 2018

I'd like your approval before merging.

would prefer keeping the class in org.apache but I understand its causing issues with 9 modules.
git rename was handled properly so an eventual look into history to understand why will be there...

I'm also curious if we're OK with using OpenJDK code from a license perspective?

not sure, with JRuby we generally avoid copy-ing code from OracleJDK which is essentially OpenJDK although licensing terms does change with different release.

@jvshahid
Copy link
Member

@Taywee why did you copy DOM2DTMdefaultNamespaceDeclarationNode.java from OpenJDK instead of using https://github.com/apache/xalan-j/blob/trunk/src/org/apache/xml/dtm/ref/dom2dtm/DOM2DTMdefaultNamespaceDeclarationNode.java. I think Xalan-J's is the original and has an Apache license.

@Taywee
Copy link
Contributor Author

Taywee commented Jun 18, 2018

@jvshahid Ignorance, probably. I saw DOM2DTMExt being almost exactly identical to the version in the OpenJDK tree and assumed that's where it had come from. My knowledge of typical licensing issues in the Java world is relatively limited (I don't work in Java much), mostly just used this as a patch to get it running in J9 on AIX. If these are issues, I'd be happy to rectify them and roll those changes into this PR.

Copy link
Member

@jvshahid jvshahid left a comment

Choose a reason for hiding this comment

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

I think this change looks ok. It doesn't add anything new that wasn't there already. We had an Apache Licensed file from Xalan that was introduced in #1604.

@flavorjones I think LICENSE-DEPENDENCIES.md cover Xalan, I don't think we need to do anything.

@kares I'm not sure how we ended down this path in the first place. I saw your comment on #1604 that Xalan isn't maintained anymore, but it looks to me like XalanJ is still maintained, or at least the mailing list & JIRA are still active (see http://mail-archives.apache.org/mod_mbox/xalan-dev/201805.mbox/browser). Is it possible to move your fixes from last year to the upstream Xalan repo so we can get rid of those patches ?

@kares
Copy link
Contributor

kares commented Jun 28, 2018

I'm not sure how we ended down this path in the first place. I saw your comment on #1604 that Xalan isn't maintained anymore, but it looks to me like XalanJ is still maintained

yeah sorry about that, I personally have a lot on my OSS plate to spend more "free" time on this, atm.
mostly did it as @flavorjones pointed out that fixing those 2 long-term issues would be very beneficial.
so it was really a 'patch till it works' job. status was seemingly dead back then - no new release in years.

@jvshahid
Copy link
Member

jvshahid commented Dec 3, 2018

Did a bit of research and I think @kares is right. Xalan-J development has stalled according to this thread. It looks like there is no one around to maintain it and release it anymore. I'm ok with merging this PR as is.

In the thread I linked to above, there is a mention of another library Saxon which has more advanced features and is better maintained. I think we should try it out.

@kares
Copy link
Contributor

kares commented Dec 3, 2018

In the thread I linked to above, there is a mention of another library Saxon which has more advanced features and is better maintained. I think we should try it out.

smt of a more recent XSLT impl to try would be awesome esp. as things are getting 'hacky' with xalan

@flavorjones
Copy link
Member

OK, I'm going to rebase this branch and, assuming it goes green, will merge it.

@jvshahid do you want me to create a new Issue to track exploration of using Saxon?

@jvshahid
Copy link
Member

jvshahid commented Dec 3, 2018 via email

@flavorjones
Copy link
Member

Created #1829 for exploring a replacement for Xalan-J.

@flavorjones
Copy link
Member

I've rebased and attempted to resolve a logical conflict at #1830. If that goes green, I'll merge and we'll ship it in the next release.

@flavorjones flavorjones added this to the next milestone Dec 4, 2018
@flavorjones
Copy link
Member

I've merged this via #1830 which was the rebase. Thanks again for submitting this, @Taywee! Will give you a shout-out in the CHANGELOG.

@flavorjones flavorjones closed this Dec 4, 2018
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.

DOM2DTMExt nextNode fails when running in J9
4 participants