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

Rsc compile task fixes #7742

Merged
merged 1 commit into from May 27, 2019

Conversation

Projects
None yet
2 participants
@cosmicexplorer
Copy link
Contributor

commented May 16, 2019

Problem

The RscCompile task is supposed to start running the zinc compile of a target once the rsc compiles of of all the target's dependencies are complete -- this is what allows for the massive parallelism from rsc in the first place. However, currently, the execution graph has the zinc compile of a target depending on the zinc compiles of all its dependencies, failing to use the rsc output at all.

This issue covered up a separate issue, because javac still can't actually use rsc output yet, and that should have caused failures, but since the zinc compiles were all scheduled after other zinc compiles, the rsc output was never being used.

These issues were fixed in #7227, but the implementation there was deemed too complex and parts were reverted without enough discussion when the "mixed compile" changes landed in master.

Solution

  • Fix execution graph dependency breakages.
  • Rename rsc-then-zinc to rsc-and-zinc, since they're supposed to happen in parallel.
  • Default the --workflow option to rsc-and-zinc.
  • Change the display of each job in the debug output displaying the dynamic execution graph (e.g. zinc[zinc-only], zinc[zinc-java], and zinc[rsc-and-zinc]).

Result

Some bugs are fixed!

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:rsc-task-fixes branch 4 times, most recently from d18b79e to af7b960 May 22, 2019

@cosmicexplorer cosmicexplorer marked this pull request as ready for review May 24, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:rsc-task-fixes branch from af7b960 to 45cc37a May 24, 2019

@stuhood stuhood added this to the 1.16.x milestone May 24, 2019

@stuhood
Copy link
Member

left a comment

Assuming I'm misreading the below, #shipit!

It looks like there is a legit unit test failure though.

}
zinc[rsc-and-zinc](scala/classpath:scala_lib) -> {}
rsc(scala/classpath:scala_dep) -> {

This comment has been minimized.

Copy link
@stuhood

stuhood May 24, 2019

Member

Does this block:

rsc(scala/classpath:scala_dep) -> {
  rsc(scala/classpath:scala_lib),
  zinc[rsc-and-zinc](scala/classpath:scala_lib)
}

...mean that using rsc for a target depends on using both rsc and zinc for a scala-only dep, or am I misreading? That would be surprising.

This comment has been minimized.

Copy link
@cosmicexplorer

cosmicexplorer May 25, 2019

Author Contributor

Replied out of line: #7742 (comment) -- the arrow might be considered to be pointing the wrong direction.

This comment has been minimized.

Copy link
@stuhood

stuhood May 25, 2019

Member

Heh, yes: definitely. Thanks!

@stuhood

This comment has been minimized.

Copy link
Member

commented May 24, 2019

(I've targeted this at 1.16.x as a post 1.16.0 item: we shouldn't cherry-pick it until after 1.16.0 is out.)

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 24, 2019

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:rsc-task-fixes branch from 45cc37a to 02cd54f May 25, 2019

@stuhood

This comment has been minimized.

Copy link
Member

commented May 25, 2019

I think that this is green with the exception of a shard that lost a race with another change landing in master? Might be good to rebase before landing.

@cosmicexplorer cosmicexplorer force-pushed the cosmicexplorer:rsc-task-fixes branch from 02cd54f to e6a4880 May 25, 2019

@cosmicexplorer

This comment has been minimized.

Copy link
Contributor Author

commented May 25, 2019

Rebased!

@cosmicexplorer cosmicexplorer merged commit c25903e into pantsbuild:master May 27, 2019

1 check failed

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

stuhood added a commit that referenced this pull request May 29, 2019

make assorted rsc task fixes (#7742)
### Problem

The `RscCompile` task is supposed to start running the zinc compile of a target once the rsc compiles of of all the target's dependencies are complete -- this is what allows for the massive parallelism from rsc in the first place. However, currently, the execution graph has the zinc compile of a target depending on the zinc compiles of all its dependencies, failing to use the rsc output at all.

This issue covered up a separate issue, because `javac` still can't actually use rsc output yet, and that should have caused failures, but since the zinc compiles were all scheduled after other zinc compiles, the rsc output was never being used.

These issues were fixed in #7227, but the implementation there was deemed too complex and parts were reverted without enough discussion when the "mixed compile" changes landed in master.

### Solution

- Fix execution graph dependency breakages.
- Rename `rsc-then-zinc` to `rsc-and-zinc`, since they're supposed to happen in parallel.
- Default the `--workflow` option to `rsc-and-zinc`.
- Change the display of each job in the debug output displaying the dynamic execution graph (e.g. `zinc[zinc-only]`, `zinc[zinc-java]`, and `zinc[rsc-and-zinc]`).

### Result

Some bugs are fixed!

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

Fix rsc compile transitive dep bug introduced in #7742 (#7825)
### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!

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

Fix rsc compile transitive dep bug introduced in #7742 (#7825)
### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!

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

Fix rsc compile transitive dep bug introduced in #7742 (#7825)
### Problem

#7742 removed the method `self._on_invalid_compile_dependency()` from `JvmCompile`, as it seemed complex and it wasn't clear if it was still needed. We were able to reproduce an error internally which made it clear that logic was still necessary, so that method was added back, along with some testing to avoid further regression.

### Solution

- Re-add `self._on_invalid_compile_dependency()`.
- Refactor rsc compile integration testing to remove the manual split between between hermetic and nonhermetic testing.
- Add a test case for a `zinc-java` target depending transitively on a scala target compiled with `rsc-and-zinc`.

### Result

The rsc compile task passes our internal CI job which compiles some code using rsc and zinc!
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.