Skip to content

[backport] Upgrade 2.12.x build to SBT 1.3.7 [ci: last-only] #8660

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

Merged
merged 12 commits into from
Feb 3, 2020

Conversation

retronym
Copy link
Member

@retronym retronym commented Jan 24, 2020

Work continues on 2.12.x for users yet to upgrade to 2.13.x. Keeping this
branch on increasingly archaic versions of SBT and MiMa carries some risk
and adds friction when switching context between branches.

This commit backports the upgade to SBT 1.2.x (#6256) and the subsequent
upgrade to 1.3.7 (#8525 #8652). This includes updating MiMa to 0.6.1 and moving
the MiMa filter lists into build.sbt and adding exclusions for
additional incompatibilties that the new version of MiMa reports
but that its too late for us to heed.

@scala-jenkins scala-jenkins added this to the 2.12.11 milestone Jan 24, 2020
@retronym retronym changed the title [backport] Upgrade 2.12.x build to SBT 1.3.7 [backport] Upgrade 2.12.x build to SBT 1.3.7 [ci: last-only] Jan 24, 2020
SethTisue and others added 3 commits January 24, 2020 10:36
1. Adapt to renaming of ivyScala

2. Be less intrusive with customization of baseDirectory

In our attempt to make forked project take on the working directory
of the root project, rather than of their module, we ended up
causing a clash in the directory used by the incremental compiler
to backup classfiles, which uses a value relative to that base.

The symptom was an intermitten error such as:

```
[error] ## Exception when compiling 0 sources to /Users/jz/code/scala-sbt-1/target/partest-extras/test-classes
[error] Could not create directory /Users/jz/code/scala-sbt-1/target/scala-2.13.0-M3/classes.bak
```

This commit adjusts ForkOptions instead to more directly
achieve our goal.

3. Heed deprecation warning in ScalaInstance tweak

4. Disable finding sources files in project base

Something has changed in SBT 1.x, and our attempt to clear the
sources for scalacheck/compile: weren't successful:

```
sbt:root> show scalacheck / Compile / unmanagedSources
[info] * /Users/jz/code/scala/test/scalacheck/array-new.scala
[info] * /Users/jz/code/scala/test/scalacheck/array-old.scala
[info] * /Users/jz/code/scala/test/scalacheck/CheckCollections.scala
[info] * /Users/jz/code/scala/test/scalacheck/CheckEither.scala
```

What changed? 4502348, earlier in this PR, removed our override
of `baseDirectory`. So sub-projects now have their own sub-directory
as the base (which is the standard SBT behaviour). If these include
source files directly, as is the case with `./test/scalacheck`, these
would be included by virtue of the long standing SBT behaviour to
let you skip the hassle of source directories for toy projects.

This commit disables that behaviour with `sourcesInBase := false`

5. Fixup OSGi test suite after baseDirectory change

6. Let SBT add api.url to the POM

Since sbt/sbt#2262, we don't need to do this.
And attempting to do it results in duplicate properties in the POM file,
which doesn't pass POM validation by artifactory.

7. Use the recommended, and working, `sbt -warn`

In favour of `--warn`, which is broken in SBT 1.x.

Early commands are ones that are run before project load, in this case,
`-warn` used to mean `-` (schedule early) and `warn` (the command to
change log level)`.

`sbt-extras` translates its `-w` option into `--warn`, or passes through
`--warn` if provided. The official SBT launcher passes through `--warn`.

`sbt -warn` still works in 1.x, but `sbt --warn` doesn't give the non-zero
error code after a failure.

```
for sbt in 0.13.17 1.1.4; do for opt in "--warn" "-warn"; do (set -x; java -jar /home/jenkins/.sbt/launchers/0.13.17/sbt-launch.jar -Dsbt.version=$sbt $opt 'eval ???'; echo $?;) done; done
+ java -jar /home/jenkins/.sbt/launchers/0.13.17/sbt-launch.jar -Dsbt.version=0.13.17 --warn 'eval ???'
scala.NotImplementedError: an implementation is missing
    at scala.Predef$.$qmark$qmark$qmark(Predef.scala:252)
...
[error] scala.NotImplementedError: an implementation is missing
[error] Use 'last' for the full log.
+ echo 1
1
+ java -jar /home/jenkins/.sbt/launchers/0.13.17/sbt-launch.jar -Dsbt.version=0.13.17 -warn 'eval ???'
scala.NotImplementedError: an implementation is missing
    at scala.Predef$.$qmark$qmark$qmark(Predef.scala:252)
...
[error] scala.NotImplementedError: an implementation is missing
[error] Use 'last' for the full log.
+ echo 1
1
+ java -jar /home/jenkins/.sbt/launchers/0.13.17/sbt-launch.jar -Dsbt.version=1.1.4 --warn 'eval ???'
[warn] sbt version mismatch, current: 1.1.4, in build.properties: "0.13.17", use 'reboot' to use the new value.
[info] Loading project definition from /tmp/scratch/project
[info] Updating ProjectRef(uri("file:/tmp/scratch/project/"), "scratch-build")...
[info] Done updating.
[info] Set current project to scratch (in build file:/tmp/scratch/)
[warn] The `-` command is deprecated in favor of `onFailure` and will be removed in a later version
[error] scala.NotImplementedError: an implementation is missing
[error]     at scala.Predef$.$qmark$qmark$qmark(Predef.scala:284)
..
[error] scala.NotImplementedError: an implementation is missing
[error] Use 'last' for the full log.
+ echo 0
0
+ java -jar /home/jenkins/.sbt/launchers/0.13.17/sbt-launch.jar -Dsbt.version=1.1.4 -warn 'eval ???'
+ echo 1
1
```

This commits changes our scripts to use the recommended `-warn`. However,
I'd say that SBT 1.x should either reject `--warn` outright or be compatible
with the old behaviour.
just keeping current
@retronym
Copy link
Member Author

retronym commented Jan 24, 2020

[error] [launcher] error during sbt launcher: sbt/sbt#4955!

[error] [launcher] sbt launcher is unable to detect the Scala version for org.scala-sbt:sbt;

[error] [launcher] this likely indicates an incomplete artifact resolution and/or corrupt boot cache (~/.sbt/boot/).

[error] [launcher] the following is the full classpath that was retrieved for org.scala-sbt:sbt:

[error] [launcher]  * /home/travis/.sbt/boot/other/org.scala-sbt/sbt/1.3.7/sbt-1.3.7.jar

sbt/sbt#4955

@retronym
Copy link
Member Author

Nuking the travis caches, I now get:

1.1.3.pom (Server returned HTTP response code: 403 for URL: https://repo1.maven.org/maven2/org/scala-sbt/launcher-interface/1.1.3/launcher-interface-1.1.3.pom) while downloading https://repo1.maven.org/maven2/org/scala-sbt/launcher-interface/1.1.3/launcher-interface-1.1.3.pom

[error]   not found: /home/travis/.ivy2/local/org.scala-sbt/launcher-interface/1.1.3/ivys/ivy.xml

[error]   not found: https://repo.scala-sbt.org/scalasbt/sbt-plugin-releases/org.scala-sbt/launcher-interface/1.1.3/ivys/ivy.xml

[error]   not found: https://repo.typesafe.com/typesafe/ivy-releases/org.scala-sbt/launcher-interface/1.1.3/ivys/ivy.xml

[error] Error downloading jline:jline:2.14.6

[error]   Not found

[error]   Not found

[error]   download error: Caught java.io.IOException: Server returned HTTP response code: 403 for URL: https://repo1.maven.org/maven2/jline/jline/2.14.6/jline-2.14.6.pom (Server returned HTTP response code: 403 for URL: https://repo1.maven.org/maven2/jline/jline/2.14.6/jline-2.14.6.pom) while downloading https://repo1.maven.org/maven2/jline/jline/2.14.6/jline-2.14.6.pom

[error]   not found: /home/travis/.ivy2/local/jline/jline/2.14.6/ivys/ivy.xml

SethTisue and others added 2 commits January 24, 2020 11:34
in order to also allow building on JDK 13+, we also disambiguate some
overloads and remove a no-longer-necesary scalaVersion override (at
the build definition level)

note that to use current sbt, we must also use current mima
and exclude some spurious issues it reports

this commit also includes some Dotty changes, contributed by
Guillaume Martres. he describes them as follows:

Fix stdlib compilation with dotty when using sbt 1.3

When we compile the standard library with dotty, we actually compile the
scala-library and dotty-library sources together in one go since we
can't in general reuse a dotty-library compiled with an old
scala-library. There were two problems with the way this was done that
were somehow masked so far (likely because of classpath pollution, but I
don't know what changed in sbt 1.3 exactly):

- Compiling the stdlib requires having all sources on the -sourcepath,
  but the sources from dotty-library itself were missing.
- the files in scalaShadowing/ in dotty-library do need to be compiled,
  the comment above the exclusion was wrong.

Additionally, I also removed the other exclusions done to files in
dotty-library because they're no longer necessary.

Co-authored-by: Guillaume Martres <[email protected]>
@dwijnand
Copy link
Member

In backporting was there any particular part you think could use more attention in reviewing? Otherwise, I'm happy with this landing if it goes green.

"error during sbt launcher" means (not generally, but what it means these days that it's popped up everywhere) I, the launcher, got 403'd by Maven Central downloading the app (sbt). That's just a risk when upgrading, but if you're not upgrading it means you're not caching the launcher jars (~/.sbt/launchers). As we're upgrading here so we just need to retry and retry the PR (ironically, given the 403 is for rate-limiting...), deleting the cache wasn't necessary (IMO).

@lrytz lrytz requested a review from dwijnand January 24, 2020 08:43
@SethTisue
Copy link
Member

SethTisue commented Jan 24, 2020

since the 2.13.x PR was merged, our Windows job on Jenkins stopped working: https://scala-ci.typesafe.com/job/scala-2.13.x-integrate-windows/1251/console

let's see if this PR has the same problem: https://scala-ci.typesafe.com/view/scala-2.12.x/job/scala-2.12.x-integrate-windows/1627/ (queued)

@dwijnand
Copy link
Member

Perhaps the changes that the community build layers on the build need adapting. Or maybe the stuff is in dbuild, in some "how to consume fresh scala" part of it (just a guess)?

@dwijnand
Copy link
Member

@SethTisue
Copy link
Member

SethTisue commented Jan 25, 2020

the Windows jobs don't use dbuild. see scripts/jobs/integrate/windows. the script builds Scala itself, it doesn't consume a Scala built elsewhere (except STARR, I mean)

@retronym
Copy link
Member Author

Here's the current blocker: sbt/zinc#571 (comment)

A warning for Scaladoc for a use case appears to have an invalid end position, and Zinc has started eagerly converting positions to lines as a convenience to BSP clients. This results in an ArrayIndexOutOfBoundsException. We don't see this on the 2.13.x branch as it seems the buggy positions are for @usecase-s which aren't needed in the 2.13.x collections docs.

@retronym
Copy link
Member Author

In backporting was there any particular part you think could use more attention in reviewing? Otherwise, I'm happy with this landing if it goes green.

The main risk of this sort of backport is failing to also backport followup changes on that branch. I've been reviewing the full history of -- build.sbt project/ on 2.13.x to look for such commits.

I'll also do a byte-for-byte comparison check on the generated artifacts (JARs, ZIPs, docs etc) once the build is green to look for any unexpected changes.

Our subprojects were sharing the `incOptions` value from the root
project, which includes the class backup directory derived from
the root project's `crossTarget` setting.

Instead, we should share the code to modify the `incOptions`.

Before:

```
sbt:root> show library/incOptions
[info] IncOptions(transitiveStep: 3, recompileAllFraction: 0.5, relationsDebug: false, apiDebug: false, apiDiffContextSize: 5, apiDumpDirectory: Optional.empty, classfileManagerType: Optional[TransactionalManagerType(backupDirectory: /Users/jz/code/scala/target/scala-2.13.0-M5/classes.bak, logger: sbt.util.Logger$$anon$2@c7ba58d)], useCustomizedFileManager: false, recompileOnMacroDef: Optional[false], useOptimizedSealed: false, storeApis: true, enabled: true, extra: {}, logRecompileOnMacro: true, externalHooks: xsbti.compile.DefaultExternalHooks@2d579733, ignoredScalacOptions: [Ljava.lang.String;@282c2bbd)
```

After:
```
sbt:root> show library/incOptions
[info] IncOptions(transitiveStep: 3, recompileAllFraction: 0.5, relationsDebug: false, apiDebug: false, apiDiffContextSize: 5, apiDumpDirectory: Optional.empty, classfileManagerType: Optional[TransactionalManagerType(backupDirectory: /Users/jz/code/scala/target/library/classes.bak, logger: sbt.util.Logger$$anon$2@c7ba58d)], useCustomizedFileManager: false, recompileOnMacroDef: Optional[false], useOptimizedSealed: false, storeApis: true, enabled: true, extra: {}, logRecompileOnMacro: true, externalHooks: xsbti.compile.DefaultExternalHooks@5e425ceb, ignoredScalacOptions: [Ljava.lang.String;@26af3265)
```

(cherry picked from commit 4d38c7d)
@@ -147,3 +147,13 @@ trait GenIterableLike[+A, +Repr] extends Any with GenTraversableLike[A, Repr] {
def zipAll[B, A1 >: A, That](that: GenIterable[B], thisElem: A1, thatElem: B)(implicit bf: CBF[Repr, (A1, B), That]): That

}

// Workaround for Scaladoc @usecase position bug scala/issues#11865
////////////////////////////////////////////////////////////////////////////////////////////////////////////////////////
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🙈

Copy link
Member Author

@retronym retronym Jan 28, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After the fix is in an we re-STARR this can go. 👀 🐵

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Fix in #8668, but we'll need the workaround until we re-STARR atop that.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The fix was merged, the de-slash-forestation can take place

@retronym
Copy link
Member Author

retronym commented Jan 28, 2020

I'm happy that we're not seeing diffs in the generated classes, JARs (other than timestamps). Scaladoc looks functionally the same although it appears that the diagram HTML output is not stable from run-to-run which makes the diffing less simple.

@retronym
Copy link
Member Author

I've also done my pass through the commits on the build and added three more backports:

  • Restore the working directory of partest, junit to the project root
  • Resort to old-school git HEAD detection in the build
  • Update Scala' build dependencies junit,jol,jackson

@retronym
Copy link
Member Author

I'd vote to defer merging this until after 2.12.11 which is scheduled in a few weeks. What do others think?

@dwijnand
Copy link
Member

As there's nothing urgent about this, sure.

@lrytz
Copy link
Member

lrytz commented Jan 31, 2020

I'd be fine merging this now. Jardiff was done - on which artifacts? I think the publishLocal artifacts include the OSGi stuff, so that would be covered. Maybe we can add a test commit that breaks bin compat to test MiMa.

@SethTisue
Copy link
Member

SethTisue commented Jan 31, 2020

It's true there's nothing urgent about this upgrade, but there's also nothing urgent or unusually high stakes about the 2.12.11 release. I think we might as well go ahead and risk the regression now; if we are doing this at all on 2.12.x, we have to risk it sometime.

TL;DR yup let's merge (though, no strong feeling either way)

@dwijnand
Copy link
Member

My thinking is we wouldn't want to have any sbt 1.3 issue affecting the 2.12 and perhaps also the 2.13 release, nor be fixed in a rush. So let's merge right after 2.12.11 and let the development cycle after that be available for fixups.

But we can also just revert this PR if it becomes problematic.

@retronym
Copy link
Member Author

retronym commented Feb 3, 2020

Okay, let's pull the trigger.

@retronym retronym merged commit b8c33ad into scala:2.12.x Feb 3, 2020
@retronym
Copy link
Member Author

retronym commented Feb 3, 2020

My jardiffing was based on the contents of build/pack after sbt setupPubishCore dist/mkPack

@SethTisue SethTisue added the internal not resulting in user-visible changes (build changes, tests, internal cleanups) label Mar 9, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal not resulting in user-visible changes (build changes, tests, internal cleanups)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants