Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Already on GitHub? Sign in to your account

Make build even more regular #2346

merged 5 commits into from Apr 4, 2013


None yet
4 participants

adriaanm commented Apr 2, 2013

One target per project (set of source files).
Always build java sources.

Easter egg: zinc support.

adriaanm added some commits Mar 28, 2013

@adriaanm adriaanm get rid of args element in staged-scalac 5dca660
@adriaanm adriaanm Regularity for build.xml: 1 output dir / project
Untangle actors, msil builds from library/compiler builds.
For further regularity, always build java files.

TODO: update IDE projects
@adriaanm adriaanm Preliminary support for zinc.
To use Zinc with the ant build:
  - install zinc and symlink the installed zinc script to ${basedir}/tools/zinc
    (${basedir} is where build.xml and the rest of your checkout resides)
  - make sure to set ZINC_OPTS to match ANT_OPTS!
  - invoke ant as `ant -Dstarr.version="2.10.1" -Dlocker.skip=1`
    (zinc does not work if locker is only classfiles, needs jars
     TODO rework the build to pack locker and build using that when using zinc?)

Mostly to enable dog fooding of incremental compilation work for now.
@adriaanm adriaanm formatting 92a1785

adriaanm commented Apr 2, 2013

review by @gkossakowski, @paulp

If this is important, why do we leave it for the user to do?


adriaanm replied Apr 2, 2013

True, I felt like pleasure-delaying one last addition to my ant skill set.

If someone has zinc on their path, they shouldn't have to do this manual step.

Is "NOT SET" something you made up or something ant understands? Either way, does it have semantics which are documented somewhere?

Agreed with @paulp.

Actually, if you scroll down you see:

+      <if><not><equals arg1="@{srcpath}" arg2="NOT SET"/></not><then>
+        <property name="args" value="@{params} -sourcepath @{srcpath}"/>
+      </then></if>

So NOT SET seems to be just a dummy value that you'll need to compare against later on. I don't know if there's an ant standard for that but a comment explaining "NOT SET" would help.

The fact that attributes got reordered made it a little bit more difficult to review this commit but I verified this change is ok.

This comment is not very illuminating. Is this a hack because we might not have anything to compile? Is that the reason why we are getting rid of <pre/> and <post/>?

What would be non-hacky way to do it?


adriaanm replied Apr 2, 2013

Correct. Alternatives would be to have an attribute to say whether javac should be invoked or not, but I figured it's more robust to just always compile it because it's so fast.

Yep, so it's not really a hack in my opinion. That's the best way to do it.


adriaanm replied Apr 2, 2013

ok, I'll fix the comment :-)

Don't forget about commit message mentioning this ;-)

Could it because the last time starr got replaced was in d3095cb which was long before 2.10.0 final and is binary incompatible with final? I believe zinc ships with precompiled compiler interface and does not compile it on demand. Right?


adriaanm replied Apr 2, 2013

I haven't double checked, but I think I saw the compiler interface being compiled in zinc as well. However, it could still explain the problem if it cached the compiled interface too eagerly. /cc @pvlugter

Zinc will compile and cache the compiler interface as needed. The compiler interface is cached under ~/.zinc/$zincVersion/compiler-interface-$scalaActualVersion-$javaClassVersion.


gkossakowski commented Apr 2, 2013

Small nitpicks but overall it goes in the right direction. 👍

@adriaanm adriaanm added a commit that referenced this pull request Apr 4, 2013

@adriaanm adriaanm Merge pull request #2346 from adriaanm/build-zinc
Make build even more regular

@adriaanm adriaanm merged commit 2914695 into scala:2.10.x Apr 4, 2013

1 check passed

default pr-checkin-per-commit Took 67 min.

@adriaanm adriaanm added a commit that referenced this pull request Apr 4, 2013

@adriaanm adriaanm Merge pull request #2347 from adriaanm/merge-2.10.x
Merge 2.10.x: #2346 edition
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment