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

8041523: Xerces Update: Serializer improvements from Xalan #7

Closed
wants to merge 1 commit into from

Conversation

kvergizova
Copy link
Contributor

@kvergizova kvergizova commented Mar 15, 2022

I'd like to backport JDK-8041523 to 8u.
This backport fixes jck8 api/xinclude test failures that occur after JDK-8037259 backport to 8u.
Аffected tests:
api/xinclude/Nist/Nist-include-38.html#Nist-include-38
api/xinclude/Nist/Nist-include-37.html#Nist-include-37
api/xinclude/Harold/harold-64.html#harold-64

Original patch from jdk9 894ae6562453 applies almost cleanly except for some copyright differences and imports reshuffling.

Tested with jdk_tier1, jdk_other (that includes javax/xml) and jck8 api/xinclude.


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8041523: Xerces Update: Serializer improvements from Xalan

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk8u-dev pull/7/head:pull/7
$ git checkout pull/7

Update a local copy of the PR:
$ git checkout pull/7
$ git pull https://git.openjdk.java.net/jdk8u-dev pull/7/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 7

View PR using the GUI difftool:
$ git pr show -t 7

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk8u-dev/pull/7.diff

@bridgekeeper
Copy link

bridgekeeper bot commented Mar 15, 2022

👋 Welcome back evergizova! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk openjdk bot changed the title Backport 38fdd0804030744b2804c35c97f843319a961799 8041523: Xerces Update: Serializer improvements from Xalan Mar 15, 2022
@openjdk
Copy link

openjdk bot commented Mar 15, 2022

This backport pull request has now been updated with issue from the original commit.

@openjdk openjdk bot added backport rfr Pull request is ready for review labels Mar 15, 2022
@mlbridge
Copy link

mlbridge bot commented Mar 15, 2022

Webrevs

@gnu-andrew
Copy link
Member

Hi,

Thanks for the backport. I don't see a link between JDK-8041523 & JDK-8037259 in the bug database. If the latter does indeed introduce a TCK failure corrected by the former, can you please link the two together? I guess that's why this wasn't spotted when JDK-8037259 was backported.

If it is indeed fixing a TCK failure introduced in 8u332, I think this should be a jdk8u-critical-request and go directly into the monojdk8u Mercurial repository once approved, rather than merging this PR.

As to the patch itself:

  • jaxp/src/com/sun/org/apache/xalan/internal/xsltc/runtime/AbstractTranslet.java copyright change is already present from JDK-8068842
  • jaxp/src/com/sun/org/apache/xalan/internal/xsltc/trax/TransformerImpl.java: same
  • jaxp/src/com/sun/org/apache/xml/internal/serializer/EmptySerializer.java: same
  • jaxp/src/com/sun/org/apache/xml/internal/serializer/ToStream.java: the structure of the diff doesn't align well with the 9u version so it's hard to tell if these changes are the same. Were any additional changes needed here? It may just be whitespace differences confusing things.

Thanks.

@kvergizova
Copy link
Contributor Author

Hi,
thanks for the comments.

I added the link and comment to JDK-8037259.

As for the jaxp/src/com/sun/org/apache/xml/internal/serializer/ToStream.java difference, probably it's better to compare with 11u git commit 38fdd08
It is similar to proposed 8u patch for ToStream.java.
Actually the only hunk that was rejected and manually reapplied due to a difference in context is #1, the copyright comment, all other hunks were applied cleanly.

@jerboaa
Copy link
Contributor

jerboaa commented Mar 16, 2022

@kvergizova This looks good to me. Please create a webrev targetting the monorepo here: https://hg.openjdk.java.net/jdk8u/monojdk8u and send an RFR with that and close this PR. It's a critical fix and we need to push it there not to the jdk8u-dev git tree. Thanks!

@gnu-andrew
Copy link
Member

Hi, thanks for the comments.

I added the link and comment to JDK-8037259.

As for the jaxp/src/com/sun/org/apache/xml/internal/serializer/ToStream.java difference, probably it's better to compare with 11u git commit 38fdd08 It is similar to proposed 8u patch for ToStream.java. Actually the only hunk that was rejected and manually reapplied due to a difference in context is #1, the copyright comment, all other hunks were applied cleanly.

Hmm, I would have thought it would be the same commit in either repository, but it does seem to have been altered in the move to git. Interesting. I guess people should use the 11u version of commits then. I'll update the 8u docs with that.

As the changes are part of that git transfer, I'm fine with your patch as is, but please rebase to monojdk8u and apply for jdk8u-critical-fix as @jerboaa suggests.

@kvergizova
Copy link
Contributor Author

@kvergizova This looks good to me. Please create a webrev targetting the monorepo here: https://hg.openjdk.java.net/jdk8u/monojdk8u and send an RFR with that and close this PR. It's a critical fix and we need to push it there not to the jdk8u-dev git tree. Thanks!

Thanks, RFR for monojdk8u: https://mail.openjdk.java.net/pipermail/jdk8u-dev/2022-March/014664.html

@kvergizova kvergizova closed this Mar 17, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backport rfr Pull request is ready for review
Development

Successfully merging this pull request may close these issues.

3 participants