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

[IntelliJ] Export only modulizable targets when in export-dep-as-jar #8812

Merged

Conversation

blorente
Copy link
Contributor

@blorente blorente commented Dec 13, 2019

Problem

As an optimization for exporting source dependencies as jars, we want to export target entries only for targets that are either:

  • Target Roots, or
  • Between target roots (e.g. if A -> B -> C and we ./pants export-dep-as-jar :A :C, we want :B to also be in the output).

Solution

Split the function generate_targets_map into three parts:
1.- We iterate over all targets to create the resource map. This is O(|all_targets|)
2.- We iterate over all targets to fill up the graph_info['libraries'] section. This is also O(|all_targets|).
3.- We calculate the printable targets (that fulfil the constraints above), and populate targets_map with entries for those. Figuring out the printable targets boils down to a call to BuildGraph.transitive_dependees_of_addresses, which is a DFS graph traversal. Populating targets_map can be quadratic, because for each modulizable target we must walk its transitive dependency graph to include all of those dependencies as libraries.

TODO before merging:

  • There are still a couple of failing tests, that will either have to be changed, or addressed before merging.
  • We need to include library entries for all the transitive deps that are not included in the final json in the targets that we print, to import them as library dependencies in IntelliJ (as opposed to module dependencies).

Result

The output json will only include the targets that fulfil the constraints defined in Problem in the targets section, as opposed to all the targets in play.

Copy link
Contributor

@wisechengyi wisechengyi left a comment

Choose a reason for hiding this comment

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

Thanks, @blorente! Looks reasonable overall. Please kindly see comments.

@blorente
Copy link
Contributor Author

Addressed comments and updated description.

@blorente
Copy link
Contributor Author

I found another problem:
(in the A->B->C->D ./pants export :A :C case) If I import a target from the IntelliJ UI (say A), and then add a project using the pants menu in IntelliJ, B will not be imported.
I think this will need some tweaking from the plugin side, very possibly to include all imported targets in calls to refresh (which should have negligible overhead)

@blorente blorente marked this pull request as ready for review December 16, 2019 14:37
@blorente
Copy link
Contributor Author

This is now ready for review.

In particular the last commit (Modify tests to accomodate for the new structure) gives a good intuition on how the output has changed. (cc\ @wisechengyi @olafurpg)

@blorente
Copy link
Contributor Author

Corresponding plugin changes: pantsbuild/intellij-pants-plugin#465

@blorente
Copy link
Contributor Author

Before change (dependencies are modules):

Screenshot 2019-12-17 at 12 08 57

After change (dependencies are libraries, only roots are modules):

Project structure:
Screenshot 2019-12-17 at 12 09 46

The library looks like this:
Screenshot 2019-12-17 at 12 08 11

@wisechengyi
Copy link
Contributor

Found some issues after a closer look.

With export-dep-as-jar examples/tests/java/org/pantsbuild/example/hello/greet/ examples/src/java/org/pantsbuild/example/hello/greet/

The target relation is correct: https://gist.github.com/wisechengyi/75e00245542efdbae26abcf009936b64#file-gistfile1-txt-L6-L8

But examples.src.java.org.pantsbuild.example.hello.greet.greet should not be there (https://gist.github.com/wisechengyi/75e00245542efdbae26abcf009936b64#file-gistfile1-txt-L13), because examples/src/java/org/pantsbuild/example/hello/greet/ is already a target root.

Neither should these two entries exist: https://gist.github.com/wisechengyi/75e00245542efdbae26abcf009936b64#file-gistfile1-txt-L121-L126 because they are target roots.

@wisechengyi
Copy link
Contributor

wisechengyi commented Dec 18, 2019

Also to be clear on the high level, given A -> B -> C -> D

$ export-dep-as-jar A
targets: {
A: { targets: [], libraries [B,C,D] }
}
libraries: {
B: // just a jar with no dep
C: // just a jar with no dep
D: // just a jar with no dep
}
$ export-dep-as-jar A B
targets: {
A: {targets: [B], libraries: []} 
B: {targets: [], libraies: [C D]}
}
libraries: {
C: // just a jar with no dep
D: // just a jar with no dep
}

@blorente blorente force-pushed the blorente/export-only-target-roots branch from 849eec1 to 7bb0f0b Compare December 18, 2019 15:20
@wisechengyi wisechengyi changed the title [IntelliJ] Export only target roots when in export-dep-as-jar [IntelliJ] Export only modulizable targets when in export-dep-as-jar Dec 21, 2019
@wisechengyi wisechengyi merged commit 43ffe73 into pantsbuild:master Dec 21, 2019
@wisechengyi wisechengyi deleted the blorente/export-only-target-roots branch December 21, 2019 08:19
wisechengyi pushed a commit that referenced this pull request Dec 21, 2019
#8812)

### Problem

As an optimization for exporting source dependencies as jars, we want to export `target` entries only for targets that are either:
- Target Roots, or
- Between target roots (e.g. if A -> B -> C and we `./pants export-dep-as-jar :A :C`, we want `:B` to also be in the output).

### Solution

Split the function `generate_targets_map` into three parts:
1.- We iterate over all targets to create the resource map. This is `O(|all_targets|)`
2.- We iterate over all targets to fill up the `graph_info['libraries']` section. This is also `O(|all_targets|)`.
3.- We calculate the modulizable targets (that fulfil the constraints above), and populate `targets_map` with entries for those. Figuring out the modulizable targets boils down to a call to `BuildGraph.transitive_dependees_of_addresses`, which is a DFS graph traversal. Populating `targets_map` can be quadratic, because for each modulizable target we must walk its transitive dependency graph to include all of those dependencies as libraries.
wisechengyi pushed a commit to pantsbuild/intellij-pants-plugin that referenced this pull request Jan 2, 2020
The follow-up change to pantsbuild/pants#8812.

### Problem

Since we will now be relying on libraries as a mechanism to express dependencies between targets, we need to process the libraries field for jvm executables as well.

### Solution

Relax the filter in `PantsLibrariesExtension` to process any scala target (not just JarLibraries).

### Result:

In combination with the pants PR above, the following UX is achieved:
pantsbuild/pants#8812 (comment)
wisechengyi pushed a commit that referenced this pull request Jan 6, 2020
#8812)

### Problem

As an optimization for exporting source dependencies as jars, we want to export `target` entries only for targets that are either:
- Target Roots, or
- Between target roots (e.g. if A -> B -> C and we `./pants export-dep-as-jar :A :C`, we want `:B` to also be in the output).

### Solution

Split the function `generate_targets_map` into three parts:
1.- We iterate over all targets to create the resource map. This is `O(|all_targets|)`
2.- We iterate over all targets to fill up the `graph_info['libraries']` section. This is also `O(|all_targets|)`.
3.- We calculate the modulizable targets (that fulfil the constraints above), and populate `targets_map` with entries for those. Figuring out the modulizable targets boils down to a call to `BuildGraph.transitive_dependees_of_addresses`, which is a DFS graph traversal. Populating `targets_map` can be quadratic, because for each modulizable target we must walk its transitive dependency graph to include all of those dependencies as libraries.
wisechengyi pushed a commit that referenced this pull request Jan 10, 2020
### Problem

When running the `export-dep-as-jar` Goal we compile all targets in the build context. This is unnecessary and can be problematic if any of the target roots have compile errors in them, because the export will fail. It is unnecessary because the target roots are being exported as source modules #8812 

### Solution

We exclude target_roots, and their dependees from the compile context.

### Result

Only targets in the build context that are not being exported as sources are compiled.
wisechengyi pushed a commit that referenced this pull request Jan 21, 2020
### Problem

When running the `export-dep-as-jar` Goal we compile all targets in the build context. This is unnecessary and can be problematic if any of the target roots have compile errors in them, because the export will fail. It is unnecessary because the target roots are being exported as source modules #8812 

### Solution

We exclude target_roots, and their dependees from the compile context.

### Result

Only targets in the build context that are not being exported as sources are compiled.
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