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 tool classpath for ./pants scalafix #6926

Merged
merged 3 commits into from Dec 14, 2018

Conversation

Projects
None yet
5 participants
@woparry
Copy link
Contributor

woparry commented Dec 13, 2018

Problem

Same as in pr #6922
In order to run custom scalafix rules that are distributed in .jar form, they must be added to scalafix's '--tool-classpath' command line argument. Currently there is no way to set this argument from pants.

Solution

Add scalafix-tool-classpath via register_jvm_tool to ./pants scalafix, with default empty classpath so you don't need to add a target if you don't want.

Result

scalafix_tool_classpath can be set in pants.ini or on the command line, and scalafix will be able to load your precompiled custom rules.

@woparry woparry changed the title Op/ad tool classpath scalafix Add tool classpath for ./pants scalafix Dec 13, 2018

@stuhood
Copy link
Member

stuhood left a comment

Thanks!

@stuhood stuhood requested a review from jsirois Dec 14, 2018

@stuhood stuhood merged commit 288c0e5 into pantsbuild:master Dec 14, 2018

1 check passed

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

This comment has been minimized.

Copy link
Member

jsirois commented Dec 14, 2018

Ah damn, you beat me by seconds - this is a bad PR:

$ cat BUILD.scalafix 
jar_library(
  name='scalafix-cli',
  jars=[
    jar(org='ch.epfl.scala', name='scalafix-cli_2.11.11', rev='0.5.2'),
  ],
)

jvm_binary(
  name='scalafix-bin',
  main='scalafix.cli.Cli',
  dependencies=[
    ':scalafix-cli',
  ]
)
$ ./pants run //:scalafix-bin -- --tool-classpath
...
15:46:59 00:02   [run]
15:46:59 00:02     [py]
15:46:59 00:02     [jvm]
15:46:59 00:02       [run]
error: Unrecognized argument: --tool-classpath

FAILURE: java scalafix.cli.Cli ... exited non-zero (8)


               Waiting for background workers to finish.
15:47:00 00:03   [complete]
               FAILURE

I was cynical enough not to trust this PR at all since It took my suggestion verbatim, except used ':' instead of os.path.sep - so I went looking for scalafix command line parsing - which due to scala - inscutable.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Dec 14, 2018

@woparry do you mind posting a fast follow that switches the default scalafix version just above your edits to whatever Twitter uses? I'm assuming your edit works with that version and that version works with this code such that the easiest thing to do is just upgrade the default in OSS Pants to support --tool-classpath.
As things stand - any attempt to use this option on a plain install of OSS Pants will blow up.

@jsirois

This comment has been minimized.

Copy link
Member

jsirois commented Dec 15, 2018

@woparry assuming you do take up the fast fix, please have a look at that version of scalafix's code that parses the option and if it uses : you're good, but if it uses java's handling for corss-platform path sep, please do said same here (even though we don't support Windows yet).

@stuhood stuhood added this to the 1.13.x milestone Dec 15, 2018

@woparry

This comment has been minimized.

Copy link
Contributor

woparry commented Dec 15, 2018

Sorry! #6938 bumps the scalafix version. I checked the scalafix code and it uses java's File.pathSeparator, so I think we want os.pathsep.

stuhood added a commit that referenced this pull request Dec 15, 2018

Bump scalafix version and use os.pathsep (#6938)
### Problem

--tool-classpath from #6926 isn't supported in the default scalafix version, and ':' was hardcoded as path separator. This meant it would break on windows, and this option wouldn't work on default pants install.

### Solution

Bump the scalafix version, and use os.pathsep.

### Result

Things will work out of the box.

illicitonion added a commit to twitter/pants that referenced this pull request Dec 17, 2018

Add tool classpath for ./pants scalafix (pantsbuild#6926)
### Problem

Same as in pr pantsbuild#6922
In order to run custom scalafix rules that are distributed in .jar form, they must be added to scalafix's '--tool-classpath' command line argument. Currently there is no way to set this argument from pants.

### Solution

Add scalafix-tool-classpath via register_jvm_tool to ./pants scalafix, with default empty classpath so you don't need to add a target if you don't want.

### Result

scalafix_tool_classpath can be set in pants.ini or on the command line, and scalafix will be able to load your precompiled custom rules.

illicitonion added a commit to twitter/pants that referenced this pull request Dec 17, 2018

Bump scalafix version and use os.pathsep (pantsbuild#6938)
### Problem

--tool-classpath from pantsbuild#6926 isn't supported in the default scalafix version, and ':' was hardcoded as path separator. This meant it would break on windows, and this option wouldn't work on default pants install.

### Solution

Bump the scalafix version, and use os.pathsep.

### Result

Things will work out of the box.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment