-
Notifications
You must be signed in to change notification settings - Fork 442
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
Upgrade sbt, scala and sbt-ci-release to avoid deprecated repo.scala-sbt.org #1566
Conversation
Depends on |
sbt-site 1.6.0 was just released with Sphinx integration |
@dwickern thanks I have seen that. Will update the PR soon |
@muuki88 This pull request is now ready., can you take a look please if you find time? Thanks! |
Also, the failing tests look unrelated to my changes |
The AppVeyor test failure looks new 🤔 |
Yeah, this one seems new, I will take a look when I find time... |
I fixed the failing Docker and GraalVM tests on the main branch if you merge/rebase again. The AppVeyor test failure doesn't seem related to your changes. I had some similar errors last time I ran those tests on my Windows machine. I suspect that your changes invalidated the cache and that's why we're seeing failures now. |
@dwickern I rebased, but the same error occurs again. Do you have a windows machine you can test what's wrong here? |
Or maybe the AppVeyor cache neeeds to be cleaned? |
I found the problem, will push soon... |
Because of sbt commit 0413727796adebdb25d9fdf70ea038723f354ee6 no security manager gets installed anymore in sbt, which causes process args to be escaped differently on Windows plattforms, see https://github.com/AdoptOpenJDK/openjdk-jdk8u/blob/master/jdk/src/windows/classes/java/lang/ProcessImpl.java#L389
Seq("-Dtest.hoge=\\[]!< >%", "\"\\[]!< >%\"", "-Dtest.huga=\\[]!<>%"), | ||
"arg #0 is [\\[]!< >%]\nproperty(test.hoge) is [\\[]!< >%]\nproperty(test.huga) is [\\[]!<>%]\nSUCCESS!" | ||
Seq("-Dtest.hoge=\\[]!< >%", "\"\\[]!< >%\"", "-Dtest.huga=\\[]!%"), | ||
"arg #0 is [\\[]!< >%]\nproperty(test.hoge) is [\\[]!< >%]\nproperty(test.huga) is [\\[]!%]\nSUCCESS!" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So, I found what made the test fail on AppVeyor running Windows.
For the record, the error message was:
error < > was unexpected at this time.
Turns out, this has nothing to do with this pull request per se, you can easily reproduce this error even with sbt-native-packager 1.9.16 on the current main branch, without this PR applied:
- You have to be on Windows
- Checkout the current
main
branch - Go to the failing scripted test's directory:
cd src/sbt-test/windows/test-bat-template
- The test succeeds with sbt 1.5.8:
sbt --sbt-version 1.5.8 -Dproject.version=1.9.16
sbt:windows-test> stage
sbt:windows-test> checkScript
# The test works
- The test fails starting with sbt 1.6.0-M1:
sbt --sbt-version 1.6.0-M1 -Dproject.version=1.9.16
sbt:windows-test> stage
sbt:windows-test> checkScript
# The test fails with above error message
I did check and debug: The process args and environment passed here are 100% the same for both sbt 1.5.8 and 1.6.0-M1, nothing changed... So why does sbt 1.6+ fail?
Before we go deeper, the technical reason why it fails is because <
and >
are handled like normal pipes starting with sbt 1.6+ and not passed as string.
So why is that?
Let's look at the Windows specific code that runs a process, this line. You can see there is a check for a SecurityManager
, and based on its existence things behave differently.
So if a SecurityManager
is available, the process will be started differently: the process args will be handled and escaped differently.
Now.. it turns out until sbt 1.5.8, sbt did install a SecurityManager
, but starting with sbt 1.6.0-M1 it does not anymore.
Look at this commit in sbt, you will see in Main.scala
the TrapExit.installManager()
got removed.
So this explains why the test started to fail with sbt 1.6.+
Now the thing is, assuming most people upgraded to sbt 1.6 or newer already anyway, this test now just got adjusted to the current real world sbt behaviour.
The reason why the scripted test fails with this pull request is because I upgraded sbt to 1.9.9 and by default the scripted tests are run with the sbt version of the main project. Like showed above however, this behaviour already surfaces in any project that uses sbt-native-packager with sbt 1.6+.
(I mean in theory I could make the scripted test here keep running with sbt 1.5.4, so I wouldn't have to adjust the test itself, but that does not make any sense IMHO).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, TIL. I agree it doesn't make sense to depend on deprecated SecurityManager
behavior.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wow, TIL.
Yeah, TIL too, took me hours to figure out what the hell is going on...
I will try 🤞 |
I started to block repo.scala-sbt.org and repo.typesafe.com in my
/etc/hosts
.See
repo.scala-sbt.org
(April 7, 2023) sbt#7202this repo could go down again.
Unfortunately you still pull in a total outdated version of sbt-ci-release which is only hosted on repo.scala-sbt.org.
Also sbt < 1.3.0 has some artifacts still hosted on that repo, so eventually I upgraded to 1.9.x+
Also there is a newer Scala patch release available already...