Skip to content

Try purging scala-xml #6480

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.

Sign up for GitHub

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

Jump to bottom
Closed
ashawley wants to merge 4 commits into from
Closed

Try purging scala-xml #6480

ashawley wants to merge 4 commits into from

Conversation

@ashawley
Copy link
Member

@ashawley ashawley commented Mar 29, 2018

Imagine the scala-xml dependency was removed from scaladoc, which see #6436.

Would it be possible to remove it as a dependency entirely from the build? Presumably, XML literal tests will fail. Anything else?

As an exercise, I've shredded scaladoc entirely so that it doesn't use scala-xml but it also doesn't do anything. I've ripped out the tests against scaladoc, as well. Although, there weren't many. So scaldoc will still compile sucessfully, and will write empty files.

@scala-jenkins scala-jenkins added this to the 2.13.0-M4 milestone Mar 29, 2018
@ashawley
Copy link
Member Author

ashawley commented Mar 29, 2018

This is only for discussion, which see scala/bug#9560

@ashawley
Copy link
Member Author

ashawley commented Mar 29, 2018

The man pages will fail to compile:

[error] scala/src/manual/scala/tools/docutil/EmitHtml.scala:10: object xml is not a member of package scala
[error]   import scala.xml.{Node, NodeBuffer, NodeSeq, XML}
[error]                ^
[error] one error found

ashawley added 2 commits March 30, 2018 09:19
@ashawley
Try purging scala-xml
760cabb 
Imagine the scala-xml dependency was removed from scaladoc.

Would it be possible to remove it as a dependency entirely from the
build?  Presumably, XML literal tests will fail.  Anything else?

As an exercise, I've shredded scaladoc entirely so that it doesn't use
scala-xml but it also doesn't do anything.  I've ripped out the tests
against scaladoc, as well.  Although, there weren't many.  So scaldoc
will still compile sucessfully, and will write empty files.
@ashawley
Remove scala-xml from HTML man page maker
4e39819
@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

Seems like a test case that is neither here-nor-there wrt scala-xml. Could be simplified?

[test] !! 1044 - run/t4124.scala                           [compilation failed]
[test] ##### Log file 'test/files/run/t4124-run.log' from failed test #####
[test] 
[test] t4124.scala:4: error: To compile XML syntax, the scala.xml package must be on the classpath.
[test]   val body: Node = <elem>hi</elem>
[test]                    ^
[test] one error found

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

Thought it was a test that could be moved to a junit test in scala-xml, but I guess it's related to the compiler's SymbolXMLBuilder.

[test] !! 1632 - run/t8253.scala                           [compilation failed]
[test] ##### Log file 'test/files/run/t8253-run.log' from failed test #####
[test] 
[test] <quasiquote>:1: error: To compile XML syntax, the scala.xml package must be on the classpath.
[test] <sample xmlns:foo={ns1}/>
[test] ^
[test] one error found

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

This also seems to be a compiler test for XML literals.

[test] !! 1696 - run/t9027.scala                           [compilation failed]
[test] ##### Log file 'test/files/run/t9027-run.log' from failed test #####
[test] 
[test] <quasiquote>:1: error: To compile XML syntax, the scala.xml package must be on the classpath.
[test] <a/>
[test] ^
[test] one error found

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

Another compiler test for XML literals:

[test] !!  875 - pos/t5154.scala                           [compilation failed]
[test] ##### Log file 'test/files/pos/t5154-pos.log' from failed test #####
[test] 
[test] t5154.scala:4: error: To compile XML syntax, the scala.xml package must be on the classpath.
[test]   def f = <z> {{3}}</z> match { case <z> {{3}}</z> => }
[test]           ^
[test] one error found

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

This test does a number of sanity checks on the REPL. It both written in a huge XML block and also contains tests of scala-xml.

[test] !! 17 - jvm/interpreter.scala                     [output differs][duration 22.12s]
[test] % diff test/files/jvm/interpreter-jvm.log test/files/jvm/interpreter.check
[test] @@ -56,8 +56,7 @@ t1513: Array[Null] = Array(null)
[test]  scala> // ambiguous toString problem from #547
[test]  
[test]  scala> val atom = new scala.xml.Atom(())
[test] -                            ^
[test] -       error: object xml is not a member of package scala
[test] +atom: scala.xml.Atom[Unit] = ()
[test]  
[test]  scala> // overriding toString problem from #1404
[test]  
[test] @@ -295,9 +294,9 @@ scala> <a>
[test]    c="c"
[test]    d="dd"
[test]  /></a>
[test] -       ^
[test] -       error: To compile XML syntax, the scala.xml package must be on the classpath.
[test] +res8: scala.xml.Elem =
[test] +<a>
[test] +<b c="c" d="dd"/></a>
[test]  
[test]  scala>

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

I think this test could be moved to a jUnit test in scala-xml's repo:

[test] !! 74 - jvm/xml05.scala                           [output differs]
[test] % diff test/files/jvm/xml05-jvm.log test/files/jvm/xml05.check
[test] @@ -1,7 +1,5 @@
[test]  
[test]  scala> <city name="San Jos&eacute;"/>
[test] -       ^
[test] -       error: To compile XML syntax, the scala.xml package must be on the classpath.
[test] +res0: scala.xml.Elem = <city name="San Jos&eacute;"/>
[test]  
[test]  scala> :quit

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

Here's a property-test related to quasiquotes that has scala-xml bindings:

[error] test/scalacheck/scala/reflect/quasiquotes/LiftableProps.scala:171: not found: value xml
[error]     implicit val liftXmlComment = Liftable[xml.Comment] { comment =>
[error]

@ashawley
Copy link
Member Author

ashawley commented Mar 30, 2018

Build succeeds with scala-xml dependency removed when:

  • scala-xml is ripped out of scaladoc and its tests
  • scala-xml is ripped out ofHTML version of scala man pages
  • It is ripped out of compiler tests for XML literals (junit and scalacheck)

@ashawley ashawley closed this Mar 30, 2018
@SethTisue
Copy link
Member

SethTisue commented Mar 30, 2018

thanks for looking into this!

offhand, I think it will be fine to relocate XML literal tests to the scala-xml repo and let the community build catch it if something about XML literal support regresses in this repo?

@SethTisue SethTisue mentioned this pull request Mar 30, 2018
Merged
@ashawley
Copy link
Member Author

ashawley commented Apr 2, 2018

Yeah, presumably the community build infrastructure would catch something like that.

@retronym
Copy link
Member

retronym commented Apr 2, 2018

Thanks to all for this effort.

We can/should retain tests for XML literals that only run the compiler as far as the parser phase, e.g. any neg test, or any pos test with -Ystop-after:parser in the corresponding .flags file. That's probably what this change has already done, I'm just making the point explicit.

@ashawley
Copy link
Member Author

ashawley commented Apr 3, 2018

The change here was considering the proposal of even ripping out those tests against XML literals, and no longer retaining them in the compiler repo. This was motivated by not having the compiler's build/bootstrap depend on scala-xml at all. Instead, the "testing" of XML literal support would be done downstream when the community build foists scala-xml (or any other Scala library that uses XML literal syntax).

@retronym
Copy link
Member

retronym commented Apr 3, 2018

My point is, testing that our parser's support for XML literals doesn't require the scala-xml library, so leaving the tests here doesn't make bootstrapping any harder, and keeps the test cases and the code-under-test colocated, which IMO is desirable. I don't feel too strongly about that, so as long as we end up with the same nett test coverage I'm happy.

@SethTisue
Copy link
Member

SethTisue commented Apr 25, 2018

ticket on resurrecting removed tests in over at scala/scala-xml is scala/scala-xml#211

@SethTisue SethTisue removed this from the 2.13.0-M4 milestone May 4, 2018
@ashawley ashawley deleted the purge-scala-xml branch June 5, 2018 16:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

4 participants

@ashawley @SethTisue @retronym @scala-jenkins