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

Upgrade sbt, scala and sbt-ci-release to avoid deprecated repo.scala-sbt.org #1566

Merged
merged 6 commits into from
Apr 11, 2024

Conversation

mkurz
Copy link
Member

@mkurz mkurz commented Jan 30, 2024

I started to block repo.scala-sbt.org and repo.typesafe.com in my /etc/hosts.

See

this 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...

@mkurz mkurz changed the title Upgrade sbt Upgrade sbt, scala and sbt-ci-release to avoid deprecated repo.scala-sbt.org Jan 30, 2024
@mkurz mkurz marked this pull request as draft January 30, 2024 15:16
project/plugins.sbt Outdated Show resolved Hide resolved
@mkurz
Copy link
Member Author

mkurz commented Feb 21, 2024

Depends on

@dwickern
Copy link
Collaborator

sbt-site 1.6.0 was just released with Sphinx integration

https://github.com/sbt/sbt-site/releases/tag/v1.6.0

@mkurz
Copy link
Member Author

mkurz commented Mar 21, 2024

@dwickern thanks I have seen that. Will update the PR soon

@mkurz mkurz marked this pull request as ready for review March 22, 2024 08:42
@mkurz
Copy link
Member Author

mkurz commented Mar 22, 2024

@muuki88 This pull request is now ready., can you take a look please if you find time? Thanks!

@mkurz
Copy link
Member Author

mkurz commented Mar 22, 2024

Also, the failing tests look unrelated to my changes

@dwickern
Copy link
Collaborator

The AppVeyor test failure looks new 🤔

@mkurz
Copy link
Member Author

mkurz commented Apr 9, 2024

The AppVeyor test failure looks new 🤔

Yeah, this one seems new, I will take a look when I find time...

@dwickern
Copy link
Collaborator

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.

@mkurz
Copy link
Member Author

mkurz commented Apr 11, 2024

@dwickern I rebased, but the same error occurs again. Do you have a windows machine you can test what's wrong here?

@mkurz
Copy link
Member Author

mkurz commented Apr 11, 2024

Or maybe the AppVeyor cache neeeds to be cleaned?

@mkurz
Copy link
Member Author

mkurz commented Apr 11, 2024

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!"
Copy link
Member Author

@mkurz mkurz Apr 11, 2024

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:

  1. You have to be on Windows
  2. Checkout the current main branch
  3. Go to the failing scripted test's directory:
cd src/sbt-test/windows/test-bat-template
  1. 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
  1. 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).

Copy link
Collaborator

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.

Copy link
Member Author

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...

@mkurz
Copy link
Member Author

mkurz commented Apr 11, 2024

@muuki88 This is totally ready to merge now 👍
Also hopefully this unblocks #1555 now so a new release can be cut 🤞

@dwickern dwickern requested a review from muuki88 April 11, 2024 16:50
@muuki88
Copy link
Contributor

muuki88 commented Apr 11, 2024

Thanks so much @dwickern and @mkurz for all the work and fixes you pushed in the last weeks 🙏🙏

I try to push a release ASAP. Or @dwickern you do it ☺️

@dwickern dwickern merged commit 612d391 into sbt:master Apr 11, 2024
15 checks passed
@dwickern
Copy link
Collaborator

I will try 🤞

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants