Conversation
|
I'll let @retronym take a look as well, since he's been involved over the whole course of this work, but the ant part LGTM -- thanks for figuring it out! /cc @szeiger / @SethTisue for sbt build follow up... |
|
I'm inclined to leave this one to @SethTisue who's been involved in the previous PRs (unless he would like to pass it on to me). The combination of this PR with the upcoming validate-test changes will break. PR validation still succeeds at the moment because validate-test rebuilds everything with ant (using the sbt-built artifacts from validate-publish-core as STARR). I see that the previous decision was to have a separate Jarlister tool and use that from Scala's build. I think a closer integration would make sense for the sbt build because we're already doing the JAR writing on our own (in |
build.sbt
Outdated
| products in Compile in packageBin ++= | ||
| (products in Compile in packageBin in forkjoin).value, | ||
| Osgi.headers += "Import-Package" -> "sun.misc;resolution:=optional, *", | ||
| packageOptions in (Compile, packageBin) += { |
There was a problem hiding this comment.
This won't have any effect because it modifies the default packaging options but scala-library.jar gets built with BND, not with sbt's built-in packaging task. The right JAR can be built by library/packageBin::packagedArtifact, you can run publishDists to build a full distribution (with output similar to the ant build's). The correct place to add this should be Osgi.bundleTask (perhaps as a new setting or by honoring packageOption there, so it can be turned on specifically for scala-library.jar without affecting other projects).
|
This change still allows the test to be exercised because it is performed before BND. So I think it is worth merging, and I will make another PR to do what you suggest. What do you think ? |
|
Actually, the tests pose a bigger problem. They do not work in this version when run from sbt: Let's take a step back and look at the ant build:
This should work fine with the changes to the ant build in this PR. Here's how it will work for sbt:
The "starr" publishing job has already been switched over to sbt, so its JAR artifacts to not get the new Manifest entries. You do not notice this yet because the test job still runs on ant and publishes a new "pack" for partest. The final publishing of the release artifacts is also done in the same way. We're close to being able to switch everything over to sbt though and we want to do this before 2.12.0 gets released. So if you want the proper artifacts in 2.12.0, it has to be done in the sbt build. The first step for jarlister is to hook into our Osgi publishing (as I described in a previous comment). Making the tests work could be more difficult. As shown above they do not work in this PR and even when you hook into the Osgi publishing in the correct way, they still won't work. The reason for this, as mentioned above, is that sbt runs partest from "quick" instead of "pack", so no packaging takes place at all. One solution would be to go back to running partest from "pack" but I'd be hesitant to do this just for the sake of these two tests here. Running it from "quick" makes for much faster turnaround times. The best solution I can think of is adding a new test project (ideally based on junit, not partest) for "pack"-based tests. This would have to be run on CI and could be run manually but wouldn't be part of a normal compile/test cycle during development. It shouldn't be too hard to add this to the sbt build but adding it also to ant would be waste of time at this point. I suggest we add the tests as disabled for now (or disable them when switching to sbt) and add the new "pack"-based testing to sbt later (after ant is gone). |
|
Stefan, it sounds good to me. I have moved the change to bundleTask. |
The goal of this change is to exercize the "manifest classpath" mechanism, meant to bring the compiler its needed classes as resources, listed in jar manifests, as opposed to files, thus enabling to use the compiler in sandboxed environments (and also the scripting engine for that matter).
| "scala.xml.*;version=\"${range;[====,====];"+versionNumber("scala-xml")+"}\";resolution:=optional," + | ||
| "scala.*;version=\"${range;[==,=+);${ver}}\"," + | ||
| "*"), | ||
| "Class-Path" -> "scala-reflect.jar scala-library.jar" |
There was a problem hiding this comment.
What is this for? I can't find any documentation for Class-Path in the BND manual and it does not make it into the MANIFEST.MF.
There was a problem hiding this comment.
It is a standard manifest attribute, note BND. It goes in scala-compiler, not scala-library. This is to be able to type a simple "jrunscript -classpath scala-compiler.jar -l scala" to launch the script engine. It is in par with what the ant build does.
There was a problem hiding this comment.
Ah, I was looking at the wrong manifest. According to http://docs.oracle.com/javase/8/docs/technotes/guides/jar/jar.html#classpath this looks rather application-specific. Does it make sense to include it in a library that gets published to a Maven or Ivy repo? A Class-Path would require a specific directory layout locally. The command line use case doesn't look very compelling to me (why use jrunscript instead of scala from the command line if you have a scala distribution installed?)
There was a problem hiding this comment.
In short : because this is what to expect from a JSR-223 compliant language. It is still useful when run from the distribution as obtained from the website. In cases where the directory layout is not right, it costs nothing to have the attribute, it will just not be used.
|
LGTM |
- Support directories in `-doc-external-doc`: It is documented as accepting a “classpath_entry_path” for the keys but this only worked for JARs and not for individual class files. When checking for external-doc mappings for a Symbol, we now find the root directory relative to a class file instead of using the full class file path. The corresponding tests for SI-191 and SI8557 are also fixed to support individual class files instead of JARs in partest. This is required for the sbt build which runs partest on “quick” instead of “pack”. - Fix version and repository handling for bootstrapping. The bootstrap `scalaInstance` can now be resolved from any repository added to the project (not just the bootstrap repositories) by using a different workaround for sbt/sbt#1872. - Workaround for sbt/sbt#2640 (putting the wrong `scalaInstance` on partest’s classpath). The required `ScalaInstance` constructor is deprecated, so we have to disable deprecation warnings and fatal warnings until there is a better fix. - Add MiMa to the sbt build (port of the old `test.bc` ant task). The sbt-mima plugin doesn’t have all the features we need, so we do it manually in a similar way to what the plugin does. Checks are done in both directions for the `library` and `compiler` projects. The base version has to be set in `build.sbt`. When set to `None`, MiMa checks are skipped. MiMa checks are run sequentially to avoid spurious errors (see lightbend-labs/mima#115). - Port the OSGi tests to the sbt build. The set of JARs that gets copied into build/osgi as bundles is a bit different from the ant build. We omit the source JARs but add additional modules that are part of the Scala distribution, which seems more correct. - Get rid up `pull-binary-libs.sh` for the sbt build. Add artifacts are resolved from the special bootstrap repository through Ivy. The special `code.jar` and `instrumented.jar` artifacts are copied to the location where partest expects them (because these paths are hardcoded in partest). Other extra JARs for partest in `test/files/lib` are referenced directly from the Ivy cache. - Move common settings that should be available with unqualified names in local `.sbt` files and on the command line into an auto-plugin. - Add an `antStyle` setting to sbt to allow users to easily enable ant-style incremental compilation instead of sbt’s standard name hashing with `set antStyle := true`. - Disable verbose `info`-level logging during sbt startup for both, `validate/test` and `validate/publish-core` jobs. Update logging is no longer disabled when running locally (where it is useful and doesn’t generate excessive output). - Pass optimization flags for scalac down to partest, using the new partest version 1.0.15\6. - Call the new sbt-based PR validation from `scripts/jobs/validate/test`. - Disable the tests `run/t7843-jsr223-service` and `run/t7933` from scala#4959 for now. We need to set up a new test project (either partest or junit) that can run them on a packaged version of Scala, or possibly move them into a separate project that would naturally run from a packaged Scala as part of the community build.
- Support directories in `-doc-external-doc`: It is documented as accepting a “classpath_entry_path” for the keys but this only worked for JARs and not for individual class files. When checking for external-doc mappings for a Symbol, we now find the root directory relative to a class file instead of using the full class file path. The corresponding tests for SI-191 and SI8557 are also fixed to support individual class files instead of JARs in partest. This is required for the sbt build which runs partest on “quick” instead of “pack”. - Fix version and repository handling for bootstrapping. The bootstrap `scalaInstance` can now be resolved from any repository added to the project (not just the bootstrap repositories) by using a different workaround for sbt/sbt#1872. - Workaround for sbt/sbt#2640 (putting the wrong `scalaInstance` on partest’s classpath). The required `ScalaInstance` constructor is deprecated, so we have to disable deprecation warnings and fatal warnings until there is a better fix. - Add MiMa to the sbt build (port of the old `test.bc` ant task). The sbt-mima plugin doesn’t have all the features we need, so we do it manually in a similar way to what the plugin does. Checks are done in both directions for the `library` and `compiler` projects. The base version has to be set in `build.sbt`. When set to `None`, MiMa checks are skipped. MiMa checks are run sequentially to avoid spurious errors (see lightbend-labs/mima#115). - Port the OSGi tests to the sbt build. The set of JARs that gets copied into build/osgi as bundles is a bit different from the ant build. We omit the source JARs but add additional modules that are part of the Scala distribution, which seems more correct. - Get rid up `pull-binary-libs.sh` for the sbt build. Add artifacts are resolved from the special bootstrap repository through Ivy. The special `code.jar` and `instrumented.jar` artifacts are copied to the location where partest expects them (because these paths are hardcoded in partest). Other extra JARs for partest in `test/files/lib` are referenced directly from the Ivy cache. - Move common settings that should be available with unqualified names in local `.sbt` files and on the command line into an auto-plugin. - Add an `antStyle` setting to sbt to allow users to easily enable ant-style incremental compilation instead of sbt’s standard name hashing with `set antStyle := true`. - Disable verbose `info`-level logging during sbt startup for both, `validate/test` and `validate/publish-core` jobs. Update logging is no longer disabled when running locally (where it is useful and doesn’t generate excessive output). - Pass optimization flags for scalac down to partest, using the new partest version 1.0.15\6. - Call the new sbt-based PR validation from `scripts/jobs/validate/test`. - Disable the tests `run/t7843-jsr223-service` and `run/t7933` from scala#4959 for now. We need to set up a new test project (either partest or junit) that can run them on a packaged version of Scala, or possibly move them into a separate project that would naturally run from a packaged Scala as part of the community build.
The goal of this change is to exercize the "manifest classpath" mechanism,
meant to bring the compiler its needed classes as resources, listed
in jar manifests, as opposed to files, thus enabling to use the compiler
in sandboxed environments (and also the scripting engine for that matter).
This is a follow-up to #4799 which was closed prematurely.