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

Re: #786: Automatically detect bundled sources in plugin dependency #954

Merged
merged 34 commits into from
Jun 6, 2022

Conversation

karollewandowski
Copy link
Contributor

Fixes: #786.

In the case when multiple source artifacts exist for a plugin, only the first one will be attached because of the IDEA issue: IDEA-292168.

This may be the case when bundled plugin contains sources - IDE sources are attached for bundled plugins so we will have plugin and IDE sources attached (attaching IDE sources was implemented in the scope of #153). I intentionally ordered plugin sources first to avoid IDEA-292168.

I was wondering whether we should configure sources when intellij.downloadSources is false and plugin is bundled, but I think it's better to add them always whenever we have them without downloading. What may look strange is that Ivy file will contain plugin's source entry in "regular" and -withSources versions. Let me know if I should change it.

Additional small cleanups:

  • added default predicate to collectJars() for cleaner client code
  • removed PluginDependency.sourcesDirectory as it is legacy and not used at all
  • added clarification about including gradle plugin project in test IntelliJ plugin project for debugging purposes

@karollewandowski
Copy link
Contributor Author

karollewandowski commented Apr 15, 2022

Windows runners tests failed due to: java.io.IOException: There is not enough space on the disk. Is it known/addressed?

@karollewandowski karollewandowski marked this pull request as draft April 15, 2022 08:28
@karollewandowski
Copy link
Contributor Author

karollewandowski commented Apr 15, 2022

Windows runners tests failed due to: java.io.IOException: There is not enough space on the disk. Is it known/addressed?

Windows runners have 14 GB of space on disk D which is used for executing actions (see: actions/runner-images#1341). We've used this limit after my changes because I added tests requiring the GoLand, and IDEA 2021.2.4 (additional 1.46 GB + 2.3 GB = 3.76 GB).
2021.2.4 is the lowest version compatible with the oldest Go plugin having /lib/src artifacts. I've checked PHP and Python plugins which do have sources as well for a potential alternative compatible with currently used 2020.1, but they don't have them in versions older than 2021.2.4.

As the solution, I will update our default IDEA version used in tests to 2021.2.4 and will adjust other tests where needed.

This issue may happen in the future again if we download more files/IDEs, but the alternative solution is not trivial (see actions/runner-images#1341) and maybe we won't need it for a longer time.

@karollewandowski
Copy link
Contributor Author

karollewandowski commented Apr 15, 2022

Windows runners tests failed due to: java.io.IOException: There is not enough space on the disk. Is it known/addressed?

Windows runners have 14 GB of space on disk D which is used for executing actions (see: actions/virtual-environments#1341). We've used this limit after my changes because I added tests requiring the GoLand, and IDEA 2021.2.4 (additional 1.46 GB + 2.3 GB = 3.76 GB). 2021.2.4 is the lowest version compatible with the oldest Go plugin having /lib/src artifacts. I've checked PHP and Python plugins which do have sources as well for a potential alternative compatible with currently used 2020.1, but they don't have them in versions older than 2021.2.4.

As the solution, I will update our default IDEA version used in tests to 2021.2.4 and will adjust other tests where needed.

This issue may happen in the future again if we download more files/IDEs, but the alternative solution is not trivial (see actions/virtual-environments#1341) and maybe we won't need it for a longer time.

I updated the default tested platform version to 2021.2.4 and we have ~2 GB margin now.
We can decrease it by 0.5 GB by getting rid of IC-14.1.4 usage in some tests, but I'm leaving it for now.

Edit:
Tests on Windows failed again.
The next week I will verify if it's not a GitHub Actions cache issue (AFAIK it may have old IDEs cached before my changes and my optimizations don't take effect until it is cleaned).

@karollewandowski karollewandowski force-pushed the 786-detecting-plugin-dependency-sources branch 2 times, most recently from e8ba6f2 to 01d07f8 Compare April 26, 2022 15:04
@@ -1,14 +1,13 @@
package org.jetbrains.intellij

import org.gradle.api.plugins.JavaPlugin
import org.gradle.testkit.runner.BuildResult
Copy link
Contributor Author

Choose a reason for hiding this comment

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

[Applies to entire file]
I cleaned the tests:

  • reduced code duplication
  • reduces code noise in testing methods, so they include actual tested context and assertions
  • added messages to assertions - before there was a lot of "expected true, but got false" messages and now there will be more precise

All existing tests and logic should be preserved.


@Test
fun `add bundled zip plugin source artifacts from src directory when localPath used`() {
val localPath = createLocalIdeIfNotExists(
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This downloads and unpacks IDEs next to the test Gradle home directory.
I created custom logic for downloading IDEs to not depend on the plugin logic (if reusing is even possible). It can be also used in other tests requiring local installations.

@karollewandowski karollewandowski marked this pull request as ready for review April 27, 2022 16:28
@karollewandowski karollewandowski force-pushed the 786-detecting-plugin-dependency-sources branch from 34cfecb to bdf0307 Compare April 27, 2022 16:30
@karollewandowski karollewandowski marked this pull request as draft May 11, 2022 12:17
@karollewandowski
Copy link
Contributor Author

Agreed with Yann to skip a feature flag for this as it would introduce unnecessary complexity.
Attaching sources depends on generating Ivy files and if we wanted to isolate the old logic of attaching IDE sources and control only attaching bundled sources, we would have to generate unique Ivy files for each context combination (no sources, IDE sources only, bundled sources only, IDE+bundled sources) and it would grow if we added new ways of attaching sources. An alternative is to control attaching sources as a whole, but we decided to skip this as it is a pretty safe feature and doesn't affect building the project itself.

I'm delaying the merge until the new version is released. I'll rebase the branch and fix failed checks after the release.

@karollewandowski karollewandowski force-pushed the 786-detecting-plugin-dependency-sources branch from 384365a to 56faffc Compare May 24, 2022 08:24
@karollewandowski karollewandowski marked this pull request as ready for review May 25, 2022 06:02
@YannCebron YannCebron self-requested a review May 25, 2022 08:11
@hsz hsz merged commit 8460273 into master Jun 6, 2022
@hsz hsz deleted the 786-detecting-plugin-dependency-sources branch June 6, 2022 08:24
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.

Automatically detect bundled sources in plugin dependency
3 participants