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

Add bootstrapper jar to compile the compile-bridge. #6462

Merged
merged 6 commits into from Sep 11, 2018

Conversation

Projects
None yet
4 participants
@blorente
Copy link
Contributor

blorente commented Sep 6, 2018

(This work was made together with @dotordogh and @illicitonion. Even if we used my fork for convenience, we programmed most of the code together. This work is based on @ity's work on the same problem.)

Problem

The _zinc_cache_dir option relies on the homedir existing to work, which is a blocker for remoting.
Discussion in #6155 .

Solution

Implement a new Scala tool, bootstrapper, that takes the relevant jars as arguments and compiles the compiler bridge. This will be called, possibly remotely, from the zinc subsystem.

Result

  • A bit of refactoring of the compiler project.
  • The creation of a bootstrapper target.

This PR is related to {}

@illicitonion
Copy link
Contributor

illicitonion left a comment

Generally looks great :)

As far as I can tell, this is probably easiest to test through integration tests when the python half lands, rather than unit testing here, which is a little sad, but not unreasonable :)

platform='java8',
)

jvm_binary(

This comment has been minimized.

@illicitonion

illicitonion Sep 7, 2018

Contributor

This should probably disappear


object Main {
/*
Assume:

This comment has been minimized.

@illicitonion

illicitonion Sep 7, 2018

Contributor

May be worth a quick:

if (args.len != 7) {
  System.err.println("Usage: zinc-bootstrapper outputPath compilerInterface...");
  System.exit(1);
}

And also fleshing out these comments to talk about where these files would come from (maybe links to their pages on maven central, or something)

Alternatively, scopt or scallop both look pretty lightweight, if we wanted to pull in one of those :)

'src/scala/org/pantsbuild/zinc/cache',
'src/scala/org/pantsbuild/zinc/options',
'src/scala/org/pantsbuild/zinc/util',
],
strict_deps=True,
platform='java8',
)

jvm_binary(

This comment has been minimized.

@illicitonion
}
}
interfaceJar
}

def interfaceId(scalaVersion: String) = CompilerInterfaceId + "-" + scalaVersion + "-" + JavaClassVersion

This comment has been minimized.

@illicitonion

illicitonion Sep 7, 2018

Contributor

Is this still used? (and variables it references)

@@ -261,9 +262,11 @@ object Settings extends OptionSet[Settings] {
prefix( "-C", "<javac-option>", "Pass option to javac", (s: Settings, o: String) => s.copy(javacOptions = s.javacOptions :+ o)),

header("sbt options:"),
// TODO: Remove the next three options

This comment has been minimized.

@illicitonion

illicitonion Sep 7, 2018

Contributor

Still TODO (and will probably lead to a non-trivial amount of code deletion :))

}
import org.pantsbuild.zinc.util.Util

object ScalaUtils {

This comment has been minimized.

@illicitonion

illicitonion Sep 7, 2018

Contributor

I could buy that this should live in CompilerUtils, or I can buy that it is worth being a standalone thing. Any strong feelings either way?

This comment has been minimized.

@blorente

blorente Sep 10, 2018

Contributor

That would mean that the :bootstrapper would depend on :compiler just for this common function, and I don't think that is a strong enough reason since the function is not intrinsic to the domain of the compiler.

@blorente blorente changed the title [WIP] Add bootstrapper jar to compile the compile-bridge. Add bootstrapper jar to compile the compile-bridge. Sep 10, 2018

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Sep 10, 2018

This is ready for review. Keep in mind that it doesn't do much without the python side.

@blorente

This comment has been minimized.

Copy link
Contributor

blorente commented Sep 10, 2018

@illicitonion illicitonion requested review from stuhood , ity and dotordogh Sep 10, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

# Copyright 2018 Pants project contributors (see CONTRIBUTORS.md).
# Licensed under the Apache License, Version 2.0 (see LICENSE).

# DOCSTART: This target is included in documentation.

This comment has been minimized.

@stuhood

stuhood Sep 10, 2018

Member

I don't think that this is true? Maybe copied from somewhere...

Regarding scopt: fine with using it for this. But consider adding a comment in src/scala/org/pantsbuild/zinc/options/OptionSet.scala recommending using scopt instead?

# DOCSTART: This target is included in documentation.
jar_library(
jars=[
jar(org='com.github.scopt', name='scopt_2.11', rev='3.7.0'),

This comment has been minimized.

@stuhood

stuhood Sep 10, 2018

Member

This should use the scala_jar helper, which will append the major/minor version automatically:

scala_jar(org='com.github.scopt', name='scopt', rev='3.7.0')
file( CompilerBridgeOpt, "file", "Specify compiler bridge sources jar", (s: Settings, f: File) => s.copy(sbt = s.sbt.copy(compilerBridgeSrc = Some(f)))),
file( CompilerInterfaceOpt, "file", "Specify compiler interface jar", (s: Settings, f: File) => s.copy(sbt = s.sbt.copy(compilerInterface = Some(f)))),
file( ZincCacheDirOpt, "file", "A cache directory for compiler interfaces", (s: Settings, f: File) => s.copy(_zincCacheDir = Some(f))),
file("-compiled-bridge-jar", "file", "Path to pre-compilede compiler interface", (s: Settings, f: File) => s.copy(compiledBridgeJar = Some(f))),

This comment has been minimized.

@stuhood

stuhood Sep 10, 2018

Member

compilede

org='org.pantsbuild',
name='zinc-scalautil',
repo=public,
publication_metadata=pants_library('Bootstrapper for The SBT incremental compiler for nailgun')

This comment has been minimized.

@stuhood

stuhood Sep 10, 2018

Member

Should fix this description.

@baroquebobcat
Copy link
Contributor

baroquebobcat left a comment

Did a quick pass and noticed a few nitpicky things. Looks pretty good.

@@ -0,0 +1,31 @@
/**
* Copyright (C) 2017 Pants project contributors (see CONTRIBUTORS.md).

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 11, 2018

Contributor

nit: 2018?

@@ -16,6 +16,7 @@ scala_library(
'3rdparty:guava',
'3rdparty:jsr305',
'src/scala/org/pantsbuild/zinc/analysis',
'src/scala/org/pantsbuild/zinc/scalautil',

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 11, 2018

Contributor

nit: sort

.validate { file =>
if (file.exists) success else failure(s"$file does not exist.")
}
.text("Path to the scala reflection library.")

This comment has been minimized.

@baroquebobcat

baroquebobcat Sep 11, 2018

Contributor

Wow. scopt is really slick!

This comment has been minimized.

@blorente

blorente Sep 11, 2018

Contributor

Yep :) It's unfortunate, because writing a CLI parser library is a thing I've wanted to do for a long time now, but with things like scopt and clap (for Rust), there's never a good reason to!

@stuhood stuhood merged commit fda7b87 into pantsbuild:master Sep 11, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@stuhood

This comment has been minimized.

Copy link
Member

stuhood commented Sep 11, 2018

These changes are published as:

org.pantsbuild#zinc-cache_2.11;0.0.5
org.pantsbuild#zinc-util_2.11;0.0.5
org.pantsbuild#zinc-scalautil_2.11;0.0.1
org.pantsbuild#zinc-bootstrapper_2.11;0.0.2
org.pantsbuild#zinc-analysis_2.11;0.0.5
org.pantsbuild#zinc-extractor_2.11;0.0.6
org.pantsbuild#zinc-compiler_2.11;0.0.8

@blorente blorente deleted the blorente:bdd/6155-scala branch Sep 13, 2018

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