Skip to content

Do not ignore XML comments when parsing#549

Merged
ashawley merged 1 commit intoscala:mainfrom
dubinsky:parse-comments
Sep 1, 2021
Merged

Do not ignore XML comments when parsing#549
ashawley merged 1 commit intoscala:mainfrom
dubinsky:parse-comments

Conversation

@dubinsky
Copy link
Contributor

@dubinsky dubinsky commented Aug 18, 2021 •
edited
Loading

fixes #508

  • set org.xml.sax.ext.LexicalHandler on the SAXParser that XMLLoader uses;
  • override LexicalHandler.comment() in FactoryAdapter so that comments are added to the element content;
  • add unit tests;
  • add excludes for the binary compatibility checks.

@dubinsky dubinsky changed the title Do not ignore XML comments when parsing (fixes #508): Do not ignore XML comments when parsing Aug 18, 2021
@dubinsky dubinsky mentioned this pull request Aug 19, 2021
Closed
@ashawley ashawley mentioned this pull request Aug 25, 2021
Merged
@SethTisue
Copy link
Member

SethTisue commented Aug 25, 2021 •
edited
Loading

@dubinsky can you rebase, in case #548 (which we just merged) causes new MiMa complaints?

@dubinsky
Copy link
Contributor Author

dubinsky commented Aug 25, 2021

can you rebase, in case #548 (which we just merged) causes new MiMa complaints?

@SethTisue done; you were right, additional excludes were needed (and added).

Now I am getting ScalaJS compatibility issues which:

  • I do not know how to address;
  • do not seem related to anything in my pull request;
  • probably are going to surface with any pull request?
[warn] Custom versioning scheme 'strict semantic versioning' for org.scala-js:scalajs-library_2.13 is the same as the versioning scheme declared in the module itself. Consider removing it from the setting libraryDependencySchemes.
[error] Incompatibilities with dependencies of scala-xml:2.0.1
[error]   org.scala-js:scalajs-library_2.13: incompatible version change from 1.6.0 to 1.7.0 (compatibility: strict semantic versioning)
[info] Fast optimizing /home/circleci/project/js/target/scala-2.13/scala-xml-test-fastopt
[info] Module scala-xml:2.0.1+10-1f87f085-SNAPSHOT is binary and source compatible with version 2.0.1
[info] Passed: Total 124, Failed 0, Errors 0, Passed 124
[error] Dependencies of module scala-xml:2.0.1+10-1f87f085-SNAPSHOT break the intended compatibility guarantees 'Compatibility.BinaryAndSourceCompatible' (see messages above). You have to relax your compatibility intention by changing the value of versionPolicyIntention.
[error] (xmlJS / versionPolicyReportDependencyIssues) Dependencies of module scala-xml:2.0.1+10-1f87f085-SNAPSHOT break the intended compatibility guarantees 'Compatibility.BinaryAndSourceCompatible' (see messages above). You have to relax your compatibility intention by changing the value of versionPolicyIntention.

@ashawley
Copy link
Member

ashawley commented Aug 25, 2021

Now I am getting ScalaJS compatibility issues

Yes, there's a compatibility problem we've introduced after merging #548 that breaks the Scala.js build for everyone. Standby.

@dubinsky
Copy link
Contributor Author

dubinsky commented Aug 25, 2021

@ashawley

Yes, there's a compatibility problem we've introduced after merging #548 that breaks the Scala.js build for everyone. Standby.

Standing by :)

@ashawley
Copy link
Member

ashawley commented Aug 28, 2021

I've opened #551 for the compatibility issue.

@dubinsky dubinsky mentioned this pull request Aug 29, 2021
Closed
@ashawley
Copy link
Member

ashawley commented Aug 31, 2021

Ok, the build should be green, now. Sorry for the delay. Please check and see if your changes work on the latest commits to main. Thank you.

@dubinsky
Do not ignore XML comments when parsing:
f92ae49 
fixes #508

- set `org.xml.sax.ext.LexicalHandler` on the `SAXParser` that `XMLLoader` uses;
- override `LexicalHandler.comment()` in `FactoryAdapter` so that comments are added to the element content;
- add unit tests;
- add excludes for the binary compatibility checks.
@dubinsky
Copy link
Contributor Author

dubinsky commented Aug 31, 2021

@ashawley

Please check and see if your changes work on the latest commits to main.

One check (SCALA=3.0.1 SCALAJS_VERSION=1.7.0 ADOPTOPENJDK=8) fails with:

downloading sbt launcher 1.5.5
# Executing command line:
java
-Dfile.encoding=UTF-8
-Xms1024m
-Xmx1024m
-Xss4M
-XX:ReservedCodeCacheSize=128m
-jar
/home/travis/.cache/sbt/boot/sbt-launch/1.5.5/sbt-launch-1.5.5.jar
clean
xmlJS/test
xmlJS/publishLocal
java.lang.NullPointerException
	at sun.net.util.URLUtil.urlNoFragString(URLUtil.java:50)
	at sun.misc.URLClassPath.getLoader(URLClassPath.java:522)
	at sun.misc.URLClassPath.getNextLoader(URLClassPath.java:494)
	at sun.misc.URLClassPath.getResource(URLClassPath.java:248)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:366)
	at java.net.URLClassLoader$1.run(URLClassLoader.java:363)
	at java.security.AccessController.doPrivileged(Native Method)
	at java.net.URLClassLoader.findClass(URLClassLoader.java:362)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:418)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:405)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:405)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:405)
	at java.lang.ClassLoader.loadClass(ClassLoader.java:351)
	at sbt.internal.XMainConfiguration.run(XMainConfiguration.java:50)
	at sbt.xMain.run(Main.scala:46)
	at xsbt.boot.Launch$.$anonfun$run$1(Launch.scala:149)
	at xsbt.boot.Launch$.withContextLoader(Launch.scala:176)
	at xsbt.boot.Launch$.run(Launch.scala:149)
	at xsbt.boot.Launch$.$anonfun$apply$1(Launch.scala:44)
	at xsbt.boot.Launch$.launch(Launch.scala:159)
	at xsbt.boot.Launch$.apply(Launch.scala:44)
	at xsbt.boot.Launch$.apply(Launch.scala:21)
	at xsbt.boot.Boot$.runImpl(Boot.scala:78)
	at xsbt.boot.Boot$.run(Boot.scala:73)
	at xsbt.boot.Boot$.main(Boot.scala:21)
	at xsbt.boot.Boot.main(Boot.scala)
[error] [launcher] error during sbt launcher: java.lang.NullPointerException

I have no clue why...

@ashawley
Copy link
Member

ashawley commented Sep 1, 2021

That build error can probably be ignored. Seems like an issue with Travis or sbt-launcher. Googling around it might be a cache issue.

@ashawley ashawley merged commit 622fadf into scala:main Sep 1, 2021
@SethTisue SethTisue mentioned this pull request Mar 22, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@ashawley ashawley ashawley approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Do not ignore XML comments when parsing

3 participants

@dubinsky @SethTisue @ashawley