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

Locate the classes directory in zinc in order to relativize classnames #7853

Merged

Conversation

Projects
None yet
2 participants
@stuhood
Copy link
Member

commented Jun 5, 2019

Problem

As described in #7850: zinc currently makes assumptions about directory layouts that aren't valid when it is being invoked via rsc.

Solution

Rather than attempting to recognize patterns, get the output directory of the compile to use for relativization.

Result

A followup PR will be able to confirm that dep-usage.jvm works under rsc.

@stuhood

This comment has been minimized.

Copy link
Member Author

commented Jun 5, 2019

Saw both #7853 and #7856, but otherwise green.

@blorente
Copy link
Contributor

left a comment

LGTM sans the nits.

Happy to see the nits fixed in a follow-up, as well.

) yield {
// strongly hold the classNames, and transform them to ensure that they are unlinked from
// the remainder of the analysis
val classNames = analysis.relations.srcProd.reverseMap.keys.toList.toSet.map(

This comment has been minimized.

Copy link
@blorente

blorente Jun 5, 2019

Contributor

I'm going to assume that this long chain worked before :)
Feel free to ask for review again if I shouldn't.

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

Yea, that's... relatively trustworthy.

val analysisDefinesClass =
for (
abstractAnalysis <- getAnalysis(classpathEntry);
analysis = abstractAnalysis.asInstanceOf[Analysis];

This comment has been minimized.

Copy link
@blorente

blorente Jun 5, 2019

Contributor

Is this always correct?

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

Yes, in practice.

abstractAnalysis <- getAnalysis(classpathEntry);
analysis = abstractAnalysis.asInstanceOf[Analysis];
compilation <- analysis.compilations.allCompilations.headOption;
singleOutput <- compilation.getOutput.getSingleOutput.asScala;

This comment has been minimized.

Copy link
@blorente

blorente Jun 5, 2019

Contributor

is this getSingleOutput always single? I think so because we're only asking for the analysis of one file, but still I want to make sure.

This comment has been minimized.

Copy link
@stuhood

stuhood Jun 5, 2019

Author Member

Yea, this is effectively a dead API in zinc.

new ClassNamesDefinesClass(classNames)
}.getOrElse {
// If we have analysis with a valid Compilation, use the classnames it refers to.
val analysisDefinesClass =

This comment has been minimized.

Copy link
@blorente

blorente Jun 5, 2019

Contributor

Could be worth annotating this with a type, it took me a while to determine that this for comprehension was single-element (an Option) and not an iterator.

@stuhood stuhood merged commit e9936fb into pantsbuild:master Jun 5, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

@stuhood stuhood deleted the twitter:stuhood/analysis-path-hardcoding-fix branch Jun 5, 2019

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

Bump to latest zinc in order to consume zinc analysis fix (#7854)
### Problem

We need to release #7853, which fixes #7850.

### Solution

Bump to the version built in #7853, and modify a test to confirm that it is fixed.

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

Bump to latest zinc in order to consume zinc analysis fix (#7854)
### Problem

We need to release #7853, which fixes #7850.

### Solution

Bump to the version built in #7853, and modify a test to confirm that it is fixed.
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.