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

Add IDEA provisioning #529

Open
wants to merge 3 commits into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
2 changes: 1 addition & 1 deletion build.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -33,7 +33,7 @@ dependencies {
implementation("com.github.javaparser:javaparser-core:3.18.0")
implementation("net.sf.jopt-simple:jopt-simple:5.0.4")
implementation("org.apache.ant:ant-compress:1.5")
implementation("commons-io:commons-io:2.6")
implementation(libs.commonIo)
implementation("org.openjdk.jmc:flightrecorder:8.0.1")
implementation("com.googlecode.plist:dd-plist:1.23") {
because("To extract launch details from Android Studio installation")
Expand Down
2 changes: 2 additions & 0 deletions gradle/libs.versions.toml
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,8 @@ groovy = "3.0.15"
spock = "2.1-groovy-3.0"

[libraries]
commonIo = "commons-io:commons-io:2.11.0"
ideStarter = "com.jetbrains.intellij.tools:ide-starter-squashed:233.13135.103"
toolingApi = "org.gradle:gradle-tooling-api:7.2"

spock-core = { module = "org.spockframework:spock-core", version.ref = "spock" }
Expand Down
1 change: 1 addition & 0 deletions settings.gradle.kts
Original file line number Diff line number Diff line change
Expand Up @@ -14,6 +14,7 @@ include("client-protocol")
include("instrumentation-support")
include("studio-agent")
include("studio-plugin")
include("ide-provisioning")

rootProject.children.forEach {
it.projectDir = rootDir.resolve( "subprojects/${it.name}")
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/gradle/profiler/CommandExec.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.gradle.profiler;

import org.apache.commons.io.output.TeeOutputStream;

import javax.annotation.Nullable;
import java.io.ByteArrayOutputStream;
import java.io.File;
Expand Down
2 changes: 2 additions & 0 deletions src/main/java/org/gradle/profiler/Logging.java
Original file line number Diff line number Diff line change
@@ -1,5 +1,7 @@
package org.gradle.profiler;

import org.apache.commons.io.output.TeeOutputStream;

import java.io.*;

public class Logging {
Expand Down
52 changes: 0 additions & 52 deletions src/main/java/org/gradle/profiler/TeeOutputStream.java

This file was deleted.

Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package org.gradle.profiler

import org.apache.commons.io.output.TeeOutputStream
import spock.lang.Specification

class AbstractIntegrationTest extends Specification {
Expand Down
24 changes: 24 additions & 0 deletions subprojects/ide-provisioning/build.gradle.kts
Original file line number Diff line number Diff line change
@@ -0,0 +1,24 @@
plugins {
id("profiler.embedded-library")
}

Copy link
Member

Choose a reason for hiding this comment

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

❓How will we use and bundle it in the gradle-profiler? Also how do you plan to publish it in reuse in gradle/gradle.

I think this two things will require some additional work.

Copy link
Member Author

Choose a reason for hiding this comment

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

  1. I thought about regular subproject usage as a dependency.
  2. In fact, we don't need to reuse it in gradle/gradle directly. gradle/gradle will invoke profiler with dedicated options for auto-provisioning.

java {
toolchain {
languageVersion.set(JavaLanguageVersion.of(17))
Copy link
Member

Choose a reason for hiding this comment

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

❓ Note: if we use Java 17 we won't be able to use it in gradle-profiler. Will we keep Java 17?

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, it seems like ide-starter was compiled using Java 17:
class file has wrong version 61.0, should be 52.0

The public API of ide-provisioning subproject is not contains any classes from ide-starter, so I think it should be possible somehow to configure toolchains in such a way, to use ide-starter with Java 17 internally, but compile ide-provisioning subproject using Java 8. Do you know how to achieve that?

Copy link
Member

@asodja asodja Jan 15, 2024

Choose a reason for hiding this comment

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

The problem is not just compiling, but also running classes compiled with Java17 on JDK < 17. So then we would need gradle-profiler to require to run only on JDK >= 17 or at least require JDK >= 17 when doing sync profiling.

Copy link
Member Author

Choose a reason for hiding this comment

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

@asodja How to do it correctly?

Copy link
Member

Choose a reason for hiding this comment

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

Good question. I believe we still want to profile with gradle-profiler on JDK8 for most scenarions, but maybe we could soften that for sync profiling.

I guess we would need to:

  1. Add a check when user tries to profile sync and they use JDK < 17 we fail
  2. Add some interface to call IDE provisioning
  3. Use multirelease jar or ServiceLoader to implement Java17 implementation

Copy link
Member

Choose a reason for hiding this comment

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

I think we need to put some more thoughts in to that

Copy link
Member Author

Choose a reason for hiding this comment

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

@asodja I think for now we can check JDK < 17 only if user enabled IDE auto-provisioning for sync. In future, when profiler will use ide-starter for sync triggering as well, we could think about it extend scope of that check, yes

Copy link
Member Author

Choose a reason for hiding this comment

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

@asodja Another "possible" option is fork ide-starter and build it from source using 8, right? But supporting a fork can be painful tho.

Copy link
Member Author

Choose a reason for hiding this comment

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

Checked out assemble of ide-starter with 8 - the main issue is using java.lang.ProcessHandle, which is was introduced in 9

Copy link
Member

Choose a reason for hiding this comment

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

I would not fork ide-starter, since that means extra work to maintain it as you mentioned. And I am sure we don't have resources for that.

Theoretically we could also just call that part of code with JDK >= 17 (so user would somehow provide a path to some JDK17 compatbile JDK), but I think having a requirement to use JDK17 for sync is ok.

I would maybe just ask on Slack if anyone has any concern regarding that (or possible if they see any other option).

}
}

repositories {
maven { url = uri("https://www.jetbrains.com/intellij-repository/releases") }
Copy link
Member Author

Choose a reason for hiding this comment

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

These repos are subject to cleanup, I hope not all of them are required. It's just copy-paste from JetBrains example repo

Copy link
Member

Choose a reason for hiding this comment

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

❓Can we clean them now?

maven { url = uri("https://cache-redirector.jetbrains.com/intellij-dependencies") }
}

dependencies {
implementation(libs.ideStarter) {
exclude(group = "io.ktor")
exclude(group = "com.jetbrains.infra")
exclude(group = "com.jetbrains.intellij.remoteDev")
}
testImplementation(libs.bundles.testDependencies)
testImplementation(libs.commonIo)
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,34 @@
package org.gradle.profiler.ide;

import kotlin.io.FilesKt;
import org.gradle.profiler.ide.idea.IDEA;
import org.gradle.profiler.ide.idea.IDEAProvider;

import java.io.File;
import java.nio.file.Path;

public class DefaultIdeProvider implements IdeProvider<Ide> {

private final IDEAProvider ideaProvider;

public DefaultIdeProvider(IDEAProvider ideaProvider) {
this.ideaProvider = ideaProvider;
}

@Override
public File provideIde(Ide ide, Path homeDir, Path downloadsDir) {
File result;
if (ide instanceof IDEA) {
result = ideaProvider.provideIde((IDEA) ide, homeDir, downloadsDir);
} else {
throw new IllegalArgumentException("Unknown IDE to provide");
}

cleanup(homeDir, downloadsDir);
return result;
}

private void cleanup(Path homeDir, Path downloadsDir) {
FilesKt.deleteRecursively(downloadsDir.toFile());
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.gradle.profiler.ide;

import org.jetbrains.annotations.NotNull;

public interface Ide {
@NotNull
String getVersion();
Copy link
Member Author

Choose a reason for hiding this comment

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

I'm doubting what data to use for providing. From one perspective version is a good and simple declaration of which release of IDE we want to get. However, there is a buildNumber as well, and it's possible to have a IDE with same version with different buildNumbers(I thinks it's the case for nightlies)

Copy link
Member

@asodja asodja Jan 15, 2024

Choose a reason for hiding this comment

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

We can always change that later, if we have a need to. So I think version is fine for now

}

Original file line number Diff line number Diff line change
@@ -0,0 +1,9 @@
package org.gradle.profiler.ide;

import java.io.File;
import java.nio.file.Path;

public interface IdeProvider<T extends Ide> {

File provideIde(T ide, Path homeDir, Path downloadsDir);
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,18 @@
package org.gradle.profiler.ide.idea;

import org.gradle.profiler.ide.Ide;
import org.jetbrains.annotations.NotNull;

public class IDEA implements Ide {
public static IDEA LATEST = new IDEA("");
private final String version;
public IDEA(String version) {
this.version = version;
}

@NotNull
@Override
public String getVersion() {
return version;
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,74 @@
package org.gradle.profiler.ide.idea;

import com.intellij.ide.starter.community.PublicIdeDownloader;
import com.intellij.ide.starter.community.model.BuildType;
import com.intellij.ide.starter.ide.IdeArchiveExtractor;
import com.intellij.ide.starter.ide.IdeDownloader;
import com.intellij.ide.starter.ide.installer.IdeInstallerFile;
import com.intellij.ide.starter.models.IdeInfo;
import org.gradle.profiler.ide.IdeProvider;
import org.jetbrains.annotations.Nullable;

import java.io.File;
import java.nio.file.Path;
import java.util.Collections;

public class IDEAProvider implements IdeProvider<IDEA> {
private final IdeDownloader ideDownloader;
private final IdeArchiveExtractor ideArchiveExtractor;

public IDEAProvider() {
this.ideDownloader = PublicIdeDownloader.INSTANCE;
this.ideArchiveExtractor = IdeArchiveExtractor.INSTANCE;
}

@Override
public File provideIde(IDEA ide, Path homeDir, Path downloadsDir) {
IdeInfo ideInfo = new IdeInfo(
"IC",
"Idea",
"idea",
BuildType.EAP.getType(),
Collections.emptyList(),
"", // latest
ide.getVersion(),
null,
null,
"IDEA Community",
info -> null
);

String version = ideInfo.getVersion().isEmpty()
Copy link
Member Author

Choose a reason for hiding this comment

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

Here and below own cache logic was implemented. Why:
IdeDownloader is able to skip downloading, but it's considering a presence of installer(like a .dmg) file.
However, I'm deleting an installer after unpacking of it, because it's quite big (1+ Gb).
At the same time, I don't want to download IDE each time. So simple heuristics was introduced - create new dir like ideaIC/latest/ and assuming that only one file (ide installation, in fact, like a .app) is there. If it there - skip downloading.

Copy link
Member

Choose a reason for hiding this comment

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

Doesn't idea starter do any caching?

Copy link
Member Author

Choose a reason for hiding this comment

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

It does. But it's relying on presence of installer(like a .dmg file). If we want to delete installers after unpacking(I guess we want), then we can't rely on it

? "latest"
: ideInfo.getVersion();

File unpackDir = homeDir
.resolve(ideInfo.getInstallerFilePrefix())
.resolve(version)
.toFile();

File unpackedIde = getUnpackedIde(unpackDir);

if (unpackedIde != null) {
System.out.println("Downloading is skipped, get " + ideInfo.getFullName() + " from cache");
return unpackedIde;
}

IdeInstallerFile installerFile = ideDownloader.downloadIdeInstaller(ideInfo, downloadsDir);
ideArchiveExtractor.unpackIdeIfNeeded(installerFile.getInstallerFile().toFile(), unpackDir);

return unpackDir.listFiles()[0];
}

@Nullable
private static File getUnpackedIde(File unpackDir) {
File[] unpackedFiles = unpackDir.listFiles();
if (unpackedFiles == null || unpackedFiles.length == 0) {
return null;
}
if (unpackedFiles.length == 1) {
return unpackedFiles[0];
}
throw new IllegalStateException("Unexpected content in " + unpackDir);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,28 @@
package org.gradle.profiler.ide

import org.apache.commons.io.output.TeeOutputStream
import org.junit.Rule
import org.junit.rules.TemporaryFolder
import spock.lang.Specification

class AbstractIdeProvisioningTest extends Specification {

private ByteArrayOutputStream outputBuffer

@Rule
protected TemporaryFolder tmpDir = new TemporaryFolder(new File("build/tmp/"))

def setup() {
outputBuffer = new ByteArrayOutputStream()
System.out = new PrintStream(new TeeOutputStream(System.out, outputBuffer))
}

protected void outputContains(String content) {
assert getOutput().contains(content.trim())
}

protected String getOutput() {
System.out.flush()
return new String(outputBuffer.toByteArray())
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,33 @@
package org.gradle.profiler.ide


import org.gradle.profiler.ide.idea.IDEA
import org.gradle.profiler.ide.idea.IDEAProvider

class IdeProviderTest extends AbstractIdeProvisioningTest {

def "can provide IDEA"() {
given:
def workDir = tmpDir.newFolder().toPath().toAbsolutePath()
def downloadsDir = workDir.resolve("downloads")
def ideHomeDir = workDir.resolve("ide")
def ideProvider = new DefaultIdeProvider(new IDEAProvider())

when:
def ide = ideProvider.provideIde(IDEA.LATEST, ideHomeDir, downloadsDir)

then:
outputContains("Downloading https://")
ide.exists()

when:
def ide2 = ideProvider.provideIde(IDEA.LATEST, ideHomeDir, downloadsDir)

then:
outputContains("Downloading is skipped, get IDEA Community from cache")
ide == ide2

and:
!downloadsDir.toFile().exists()
}
}