Skip to content

Conversation

@liucijus
Copy link
Collaborator

Description

Added setup_scala_toolchain macro to simplify scala toolchain setup.

Motivation

To configuration of custom scala toolchain one needs to understand implementation details like dependency provider ids, which makes it a complicated task. We can encapsulate those details under a macro.

@liucijus liucijus requested a review from simuons as a code owner October 31, 2022 13:09
scala_library_classpath_provider = "%s_scala_library_classpath_provider" % name
scala_macro_classpath_provider = "%s_scala_macro_classpath_provider" % name

print("XML", scala_xml_provider)
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is this really needed?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks, for catching, @simuons!


def setup_scala_toolchain(
name,
scala_xml_deps,
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

nit: why some attribute names ends with deps and others with classpath? What's the distinction?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

classpath is what historically was used on the toolchain, other names came when dep providers were introduced. How I see it: classpath is implicit deps on the classpath, and deps is tool related deps. But this naming assumption does not even hold with the current codebase (eg. compiler is tool, but has classpath naming). Not sure what to do about it. It's a good time to improve those names at least on the macro side. Naming suggestions are more than welcome. @simuons, WDYT?

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't really have good suggestion. I wasn't aware of this distinction about classpath and tools. My initial thought was "all those deps go to the classpath at some point so why the distinction". The question probably is wether users should set 'internal' deps for things like //scala/support:test_reporter but this is out of scope for this PR. So I'm ok with state as it is now.

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

After thinking a bit more, I'll change it to use default providers for tools deps. This should give us more room to do future refactorings. Eg. move scala_xml dep to testing toolchain.

@liucijus
Copy link
Collaborator Author

@simuons I have made scala xml and parser combinators deps to be default if not specified

@liucijus liucijus force-pushed the simplify-scala-toolchain-setup branch from fd8d753 to 6b522fc Compare December 8, 2022 09:37
@simuons simuons merged commit 11a2dd5 into bazel-contrib:master Dec 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants