-
Notifications
You must be signed in to change notification settings - Fork 304
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
Don't build unnecessary jars. #5515
Conversation
Hi!
Would you be willing to run some benchmarks on open source projects and provide the results here? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The implementation looks good to me, and I think it's unlikely to interfere with indexing. However, as @agluszak said, given that this changes a lot of the core resolution, it would be interesting to run the same benchmark in other projects (such as this repository, for instance) before merging, to make sure it's not a regression.
and
I can provide a rough count of how many Jars would be ignored however I don't know how interesting that number is, instead what would be more interesting is to measure the impact this may have on indexing and code-completion time but unfortunately Intellij IDE itself nor this plugin provide an easy way to get those numbers |
If the change is risky it would be good to at least provide some way to recover from it, ex. via a feature flag. Otherwise users will be blocked until we push a hotfix release |
Can we try to incrementally roll it out?, I feel like we have been adding a lot of feature flags and not all of them are documented so not a lot of users get to try the features they protect. |
Sure, how to do it?
Sorry, I know I was supposed to document the flags 😅 putting it on my priority list. |
We can use |
Follow up on how we can select between the old and new behaviour in the aspect, we can use --aspects_parameters to pass a boolean value to the aspect based on the experiment value (i.e. enabled or not) and select the one of the 2 approaches in the aspect based on this boolean. We need to add a boolean attribute in the aspect definition, can be called |
1ec913d
to
1d786c2
Compare
aspect/testing/tests/src/com/google/idea/blaze/aspect/java/dependencies/DependenciesTest.java
Outdated
Show resolved
Hide resolved
PTAL @mai93 @tpasternak |
…endencies/DependenciesTest.java
…tring' and use the 'values' restriction.
bf1696c
to
25b5c65
Compare
@@ -58,10 +58,8 @@ public void testJavaBinary() throws Exception { | |||
testRelative(intellijInfoFileName("foo"))); | |||
assertThat(getOutputGroupFiles(testFixture, "intellij-resolve-java")) | |||
.containsAtLeast( | |||
testRelative("libfoolib.jar"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why these jars are no longer expected, is optimization enabled for this test? same for DependenciesTest
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Reverted these changes, they were traces to when optimization was enabled by default.
it looks good to me, I can merge it if none else wants to review cc @tpasternak @agluszak |
This option isn't supported on the version of Bazel we use. How do you disable it for a project? |
what feature is not supported in the Bazel version you use? @nkoroste mentioned that this experiment can be disabled by disabled it in the IntelliJ experiments file as follows:
|
Any Bazel version older than when this commit landed will fail:
|
Bazel 5 will be EOL in September. https://blog.bazel.build/2020/11/10/long-term-support-release.html @aschott-looker are there any chances you could upgrade to bazel 6? |
https://bazel.build/release says it's EOL in Jan 2025. |
@tpasternak We definitely want to upgrade to a more recent version of Bazel but it's not feasible in the short term. So I was hoping for a quick workaround. Thanks for creating the issues above! |
the hotfix is on its way #6653 |
Checklist
Please note that the maintainers will not be reviewing this change until all checkboxes are ticked. See
the Contributions section in the README for more
details.
Discussion thread for this change
Issue number: #1785
Description of this change
reopened #1786 - Don't build the class jar if source jars are available, as we can use that in conjunction with the interface jar to index the code instead.
For our app it reduces
~30%
of the Jars the IDE will index (from14484
to10072
)As per PR review request I put this feature behind
optimize_building_jars
experiment which is enabled by default. In case an issue is found with this feature this experiment can be disabled by disabled it in the IntelliJ experiments file as follows: