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

Migrate zinc compiler to scopt #6803

Merged
merged 2 commits into from Jun 7, 2019

Conversation

Projects
None yet
2 participants
@illicitonion
Copy link
Contributor

commented Nov 22, 2018

Fixes #6487

I'm not 100% convinced I prefer this, but I do like the idea of deleting an entire custom options parser...

@illicitonion illicitonion requested review from stuhood and blorente Nov 22, 2018

@illicitonion

This comment has been minimized.

Copy link
Contributor Author

commented Nov 22, 2018

I'm expecting tests to fail because the python changes need to happen after the scala is published and the version bumped, but wanted to show the full extent of changes to get feedback

@illicitonion illicitonion force-pushed the twitter:dwagnerhall/zinc-compiler/scopt branch from 1fcdbea to 3d19f12 Nov 23, 2018

@stuhood
Copy link
Member

left a comment

Looks good, thanks!

fixedArgs(i - 1) = "--classpath"
}
// Old versions of this binary used :s not ,s as list separators.
// Accept their input.

This comment has been minimized.

Copy link
@stuhood

stuhood Nov 26, 2018

Member

Backwards compatibility is not a strict concern here... 99% of consumers use the built-in zinc version, and Twitter only overrides the version to add some debugging tools to a classpath that matches the built-in version.

val fixedArgs = args.flatMap { arg =>
arg match {
case x if x.startsWith("-C") || x.startsWith("-S") => {
val tup = arg.splitAt(2)

This comment has been minimized.

Copy link
@stuhood

stuhood Nov 26, 2018

Member

Can unpack with val Array(x, y) = arg.splitAt(2).

@stuhood stuhood self-assigned this Jun 6, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jun 6, 2019

I'm going to rebase this and split out the python portion.

@stuhood stuhood force-pushed the twitter:dwagnerhall/zinc-compiler/scopt branch from 3d19f12 to 25819bb Jun 6, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented Jun 7, 2019

This passes JVM tests, and a bunch of local sanity testing. Going to merge to publish and run the whole suite. Hold onto your butts.

@stuhood stuhood merged commit 99cd974 into pantsbuild:master Jun 7, 2019

0 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/travis-ci/push The Travis CI build could not complete due to an error
Details

@stuhood stuhood deleted the twitter:dwagnerhall/zinc-compiler/scopt branch Jun 7, 2019

stuhood added a commit that referenced this pull request Jun 7, 2019

Use argfile support in zinc and rsc (#7868)
### Problem

In hermetic and subprocess environments, large targets can fail with "argument list too long" errors. As of #6803, `zinc` supports `@argfile`s, and `rsc` has supported them for a while. 

### Solution

Use `@argfile` support in all environments (to minimize variability).

### Result

subprocess and hermetic modes scale to larger contexts.

stuhood added a commit that referenced this pull request Jun 7, 2019

Use argfile support in zinc and rsc (#7868)
### Problem

In hermetic and subprocess environments, large targets can fail with "argument list too long" errors. As of #6803, `zinc` supports `@argfile`s, and `rsc` has supported them for a while. 

### Solution

Use `@argfile` support in all environments (to minimize variability).

### Result

subprocess and hermetic modes scale to larger contexts.

cattibrie added a commit to cattibrie/pants that referenced this pull request Jun 19, 2019

Use argfile support in zinc and rsc (pantsbuild#7868)
### Problem

In hermetic and subprocess environments, large targets can fail with "argument list too long" errors. As of pantsbuild#6803, `zinc` supports `@argfile`s, and `rsc` has supported them for a while. 

### Solution

Use `@argfile` support in all environments (to minimize variability).

### Result

subprocess and hermetic modes scale to larger contexts.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.