Update jline to 3.24.1

[scala-steward]

About this PR

📦 Updates org.jline:jline from 3.22.0 to 3.24.1

Usage

✅ Please merge!

I'll automatically update this PR to resolve conflicts as long as you don't change it yourself.

If you'd like to skip this version, you can just close this PR. If you have any feedback, just mention me in the comments below.

Configure Scala Steward for your repository with a .scala-steward.conf file.

Have a fantastic day writing Scala!

⚙ Adjust future updates

Add this to your .scala-steward.conf file to ignore future updates of this dependency:

updates.ignore = [ { groupId = "org.jline", artifactId = "jline" } ]

Or, add this to slow down future updates of this dependency:

dependencyOverrides = [{
  pullRequests = { frequency = "30 days" },
  dependency = { groupId = "org.jline", artifactId = "jline" }
}]

^{labels: library-update, early-semver-minor, semver-spec-minor, commit-count:1}

[stevedlawrence]

I'm not sure we're ever going to be able to update jline because it conflicts with SBT dependencies. The only suggestion by SBT devs is to fork our tests, which I believe causes too much of a performance hit and really slows down testing.

Maybe we move all CLI tests to daffodil-test-integration, and enable forking for just that, so we at least only take the performance hit on integration tests?

Maybe we could switch to a different library? Jline is only used for the CLI debugger for thing slike command autocompletion and history, we don't need anything too fancy.

Any other options?

[stevedlawrence]

For reference, PR #986 has discussions about jline 3.23.x and the issues it causes with sbt.

[tuxji]

One option is to move all CLI tests to daffodil-test-integration and enable forking for just those tests.

Another option is to shade the jline dependency (change its package name) so that we don't get any conflict with sbt's own jline. See scala/bug#9339.

[stevedlawrence]

I would prefer to not have to shade anything. It would be nice if SBT did that since it's the thing with a broken class loader.

I think moving tests to daffodil-test-integration and having them fork is probably the best bet. And we should only need to move the CLI debugger tests, since they are the only thing that use jline. I'll give that a shot and see how it goes.

[stevedlawrence]

With the updates to change debug tests to fork, we now get a number of failures on windows. I've tracked down the cause to these lines:

https://github.com/apache/daffodil/blob/main/daffodil-cli/src/main/scala/org/apache/daffodil/cli/debugger/CLIDebuggerRunner.scala#L61-L66

These lines caused all of our CLI tests that used jline to use a DumbTerminal because they didn't use normal stdin/stdout. But by changing these tests to fork, they now use normal stdin/stdout and so use the TerminalBuilder to get a jline terminal. For whatever reason, the terminal that jline returns causes random failures. I haven't yet figured out why, if there is a workaround, or if there's an alternative way to detect that we are doing integration tests and should use a DumbTerminal.

Interestingly, when these CLI tests fail, jline outputs:

WARNING: Unable to create a system terminal, creating a dumb terminal

So it seems like it's using the same DumbTerminal. Maybe it's configured in a slightly different way, or its terminal detection is somehow breaking things...

[stevedlawrence]

I found some jline properties that can be set in the debugger tests to force a dumb terminal, even when using stdout/stdin. This fixed most the tests on windows. I also found that windows does not run with file.encoding=UTF-8, which some debugger tests require. Setting that fixed the remaining tests. And this also required adding a way to set those properties in the DAFFODIL_JAVA_OPTS environment variable with the runCLI method.

This is now ready for review, and should let us keep jline up to date with upstream.

[tuxji]

+1

[stevedlawrence]

@mbeckerle, we'll should probably get another +1 from someone other than me since I ended up making a number of changes to get jline working

[mbeckerle]

+1

Just some additional commentary to understand the need for this change better. The changes themselves all look good.

[mbeckerle]

Why is this only an issue for CLI debugger tests? Why doesn't this same issue plague us everywhere?

Can you add an explanation about that? Otherwise this reasoning seems compelling as to a need to always do the forking for all testing.

Also, why did the change of jline version cause all this need?

[stevedlawrence]

Also, why did the change of jline version cause all this need?

When SBT runs tests it makes two different versions of jline available via its class loader--one is the version we depend on and the other is what SBT uses for its console stuff, which lags behind the version we use. In some cases, the SBT test class loader finds one class from one jline version but a different class from the other version. And jline made changes to one of the classes so that the different versions can't be used together. I think we were just lucky in the past and the different classes were compatible. By forking, SBT's jline is no longer on the classpath, so this issue doesn't show up.

It seems like a pretty clear bug that SBT is putting its version of jline on the classpath, but we've reported it and they don't seem inclined to fix it.

We've had similar issues with things like test IBM DFDL, where we ended up with two different versions of ICU4J on the classpath, one needed by Daffodil and one needed by IBM DFDL. Java's classpath issues strike again.

Why is this only an issue for CLI debugger tests?

The CLI debugger is the only thing in Daffodil that uses jline. If the debugger is never triggered by a test, then the class loader never tries to find, load, and use those incompatible classes from different jars.

I'll update the comment to make this issue a bit more clear.