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

remove complicated invalid dep check in rsc compile #8131

Merged

Conversation

@cosmicexplorer
Copy link
Contributor

commented Aug 1, 2019

Problem

There are several lines of code in RscCompile and JvmCompile relating to the overridden _on_invalid_compile_dependency() method, with outstanding TODOs to document how they work and why they are necessary. We are actually able to remove this entirely.

This may address #8109 -- I haven't been able to repro the issue yet.

Solution

  • Remove _on_invalid_compile_dependency() and replace it with the solution described in #8109 (comment).

Result

The rsc compile invalidation is much less confusing!

@cosmicexplorer cosmicexplorer requested review from stuhood, illicitonion and ity Aug 1, 2019

@stuhood

stuhood approved these changes Aug 1, 2019

Copy link
Member

left a comment

Thanks! If this doesn't represent a performance regression for cold/noop builds, then it's definitely a significant improvement! Would be good to benchmark before landing.

@stuhood stuhood added this to the 1.19.x milestone Aug 1, 2019

@ity

This comment has been minimized.

Copy link
Contributor

commented Aug 5, 2019

@stuhood - I will take this over from @cosmicexplorer to land.

  • just pushed a change to re-enable the failing test for the mixed-compile-workflow-test that this change should fix
  • will evaluate the perf benchmarks

@ity ity force-pushed the cosmicexplorer:remove-invalid-compile-dependency-search branch from eac928e to bedca8a Aug 6, 2019

@ity

This comment has been minimized.

Copy link
Contributor

commented Aug 7, 2019

travis CI just went green on this, merging it now

@ity ity merged commit d47cc02 into pantsbuild:master Aug 7, 2019

1 check passed

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

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.