Skip to content
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

[WIP] Phase format #874

Closed
wants to merge 8 commits into from
Closed

Conversation

borkaehw
Copy link
Contributor

@borkaehw borkaehw commented Nov 7, 2019

Description

This is an example of how to add custom phases without changing default rules behavior. This phase introduces formatting to bazelbuild/rules_scala.

Copied most of the code from https://github.com/higherkindness/rules_scala

Motivation

Formatting is a nice-to-have feature.

@borkaehw
Copy link
Contributor Author

borkaehw commented Nov 7, 2019

Force pushed to fix format.

@borkaehw borkaehw changed the title Phase format [WIP] Phase format Nov 7, 2019
@borkaehw
Copy link
Contributor Author

borkaehw commented Nov 7, 2019

We are happy to figure out attribution from higherkindness/rules_scala before merging, but wanted to provide an example of the phases architecture.

@psilospore
Copy link

psilospore commented Dec 27, 2019

Hey @borkaehw,

I have a branch off of this and I'm trying to implement a bloop integration. I did some work off of higherkindness/rules_scala and was referenced to here. So now I'm trying to port over my code.

I read the documentation on adding a phase here: https://github.com/bazelbuild/rules_scala/pull/865/files#diff-3152e63cc3552788493d47c5dde927a2 and I've looked through this PR and mostly referenced the work done for scalafmt to write a bloop phase.

First I wanted to first check if I'm doing the right approach here: https://github.com/lucidsoftware/rules_scala_bazelbuild/compare/phase-format...psilospore:bloop-phase?expand=1#diff-60c470b96b12437fced2f68a105809dd

Secondly, I am having a bit of trouble with indirect dependencies. At runtime I've been getting some class not found exceptions.

I added dependencies like this:

    _scala_maven_import_external(
        name = "bloop-launcher",
        artifact = "ch.epfl.scala:bloop-launcher_2.12:1.2.5",
        licenses = ["notice"],
        server_urls = maven_servers,
    )

    native.bind(
        name = "io_bazel_rules_scala/bloop/blooplauncher",
        actual = "@bloop-launcher",
    )

Then used it like this


scala_binary(
    name = "bloop",
    srcs = glob(["bloop/BloopRunner.scala"]),
    main_class = "io.bazel.rules_scala.bloop.BloopRunner",
    visibility = ["//visibility:public"],
    deps = [
        "//src/java/io/bazel/rulesscala/worker",
        "@scalafmt//:net_sourceforge_argparse4j_argparse4j", #TODO move
        "//external:io_bazel_rules_scala/bloop/bsp4j",
        "//external:io_bazel_rules_scala/bloop/bloopconfig",
        "//external:io_bazel_rules_scala/bloop/blooplauncher",

But then when I use something in blooplauncher it gives me this:

bazel run //scala/bloop:bloop
...
Exception in thread "main" java.lang.NoClassDefFoundError: com/zaxxer/nuprocess/internal/BasePosixProcess

It seems like I'm doing something wrong and I'm pretty new to bazel so I suspect I am.

@ittaiz
Copy link
Member

ittaiz commented Dec 29, 2019 via email

@psilospore
Copy link

psilospore commented Jan 5, 2020

Thanks @ittaiz I was able to figure out how to use bazel-deps to have my bloop runner working.

Are you certain BasePosixProcess comes from blooplauncher jar?

Seems like it does here's the a generated file from bazel-deps

scala_import(
    name = "bloop_launcher",
    exports = [
        "//3rdparty/jvm/com/zaxxer:nuprocess",

@psilospore
Copy link

psilospore commented Jan 5, 2020

I want to make sure I'm understanding this right. Right now I'm trying to follow the scalafmt example where I just add an additional final phase:

def _add_phase_bloop_singleton_implementation(ctx):
    return [
        _ScalaRulePhase(
            phases = [
                ("$", "", "bloop", _phase_bloop)
                #TODO plan is to make it a phase at the end then replace it later. Only using the output from phases before compile.
                #("=", "compile", "bloop", _phase_bloop),

            ],
        ),
    ]

Where I will generate bloop configs and test if I'm able to make a compile BSP request successfully to bloop. Once that works I'm thinking I would do a replace on the compile phase.

                ("=", "compile", "bloop", _phase_bloop),

Doing that means I would need to return the same struct as compile which seems to be in this shape:

    return struct(
        class_jar = ...,
        coverage = ...,
        full_jars = ...,
        ijar = ...,
        ijars = ...,
        rjars = ...,
        java_jar = ...,
        source_jars = ...,
        merged_provider = ...,
    )

Then I can do something like this except with bloop:

   load("//scala/scalafmt:scala_with_scalafmt.bzl", "scala_binary", "scala_library", "scala_test")

Does that sound like I'm headed towards the right direction?

@borkaehw
Copy link
Contributor Author

borkaehw commented Jan 6, 2020

@psilospore Hi there, sorry for late response since I just got back from a long vacation.

add an additional final phase

Just keep in mind phase_final will always be the final phase.

Doing that means I would need to return the same struct as compile which seems to be in this shape

Theoretically yes, otherwise phases after phase_compile may miss something.

Then I can do something like this except with bloop

Yes, if it's merged in. You may also keep this work in your private workspace without opening a PR.

We have made a lot of changes on phase architecture. This phase format PR is very outdated. I would branch off of master and work from there. I will do the same to re-open phase format PR if we decide to move forward.

@borkaehw
Copy link
Contributor Author

borkaehw commented Jan 8, 2020

This PR will be replaced by #912.

@borkaehw borkaehw closed this Jan 8, 2020
@psilospore
Copy link

Thanks for letting me know I'm headed towards the right direction! @jvican and I would probably be interested in getting this merged in eventually vs having it in another repo.

I'll branch off of master.

@borkaehw borkaehw deleted the phase-format branch January 8, 2020 07:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants