Fixes #91 - Moved instrumentation logic out of the compiler #99

Merged
merged 1 commit into from Oct 17, 2012

Conversation

Projects
None yet
2 participants
Owner

dotta commented Oct 16, 2012

review by @dragos

Following up on scala/scala@4c7127d,
this commit moves the instrumentation logic out of the compiler project.

This has huge benefits as it will allows us to fix instrumentation bugs
without having to wait for a new release of the compiler.

This can be merged only after scala/scala#1504 is merged and a new Scala 2.9.3 SNAPSHOT is produced, or the worksheet for Scala 2.9 won't build.

object WorksheetPlugin {
@volatile var plugin: WorksheetPlugin = _
- private val PLUGIN_ID = "Worksheet Plugin"
+ private val PluginId = "org.scalaide.worksheet"
@dotta

dotta Oct 16, 2012

Owner

make this final

Fixes #91 - Moved instrumentation logic out of the compiler
Following up on scala/scala@4c7127d,
this commit moves the instrumentation logic out of the compiler project.

This has huge benefits as it will allows us to fix instrumentation bugs
without having to wait for a new release of the compiler.
Owner

dotta commented Oct 17, 2012

@dragos Please, can you have a look at it (so that I can proceed with backporting this to 0.1.x branch).

</artifactItem>
</artifactItems>
+ <outputDirectory>${project.build.directory}/lib</outputDirectory>
@dragos

dragos Oct 17, 2012

Owner

Is this intended to be moved outside the <artifactItem> element? (I don't know much about this, just asking).

@dotta

dotta Oct 17, 2012

Owner

If you put it within the <artifactItem>, then it will only apply to that specific dependency. In this case it doesn't make any difference, but I still think it's better to have it outside as that folder will be good for any additional dependency we may add in the future...

@dragos

dragos Oct 17, 2012

Owner

Cool, thanks for the explanation.

@@ -0,0 +1,53 @@
+package scala.tools.nsc.interactive
@dragos

dragos Oct 17, 2012

Owner

Do we need to keep the same package? Split packages are a huge source of problems in OSGi, so I'd stay away from them if possible.

@dotta

dotta Oct 17, 2012

Owner

Yes, we need it in the same package because my PR on the Scala project made global.interruptsEnabled package private, and not public. I did it this way because I wanted to make it hard for other plugins to access interruptsEnabled.

As for possible incorrect behavior, I've built a worksheet locally and installed and it seems fine...

@dotta

dotta Oct 17, 2012

Owner

But I can create an additional PR for RC2 and make that field public. WDYT?

@dotta

dotta Oct 17, 2012

Owner

By the way, split packages may be a real issue the moment you export them twice (http://wiki.osgi.org/wiki/Avoid_Split_Packages), which is not happening in the worksheet plugin, i.e., I'm not exporting that package.

@dotta

dotta Oct 17, 2012

Owner

And http://eclipsesource.com/blogs/2008/08/22/tip-split-packages-and-visibility/ gives a workaround for when you do want to export split packages twice (or more).

@dragos

dragos Oct 17, 2012

Owner

Ok, seems like you carefully weighted alternatives and this is the best compromise. So LGTM 👍 !

+
+// Temporarily named `ScratchPadMaker2` to avoid collision with the existing `ScratchPadMaker` in the
+// compiler (which is located in the same package)
+private[interactive] trait ScratchPadMaker2 {
@dragos

dragos Oct 17, 2012

Owner

..so it would be nice if this could be moved to another package altogether.

@dotta

dotta Oct 17, 2012

Owner

Same comment as above.

dotta added a commit that referenced this pull request Oct 17, 2012

Merge pull request #99 from dotta/issue/move-worksheet-support-in-#91
Fixes #91 - Moved instrumentation logic out of the compiler

@dotta dotta merged commit 1eee867 into scala-ide:master Oct 17, 2012

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment