-
Notifications
You must be signed in to change notification settings - Fork 156
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
Pass idea.properties to studio #507
Conversation
import java.util.Map; | ||
import java.util.Set; | ||
import java.util.TreeSet; | ||
import java.util.*; |
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.
I'm personally don't like star imports, but it seems project is using it
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.
I think we just don't have that strict IDE configuration for this project
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.
Looks good! I just added a comment about creating idea.properties
in config dir. I think we could move that to the same folder as studio.vmoptions
, so we can override these options always and not only when sandbox is present.
def vmOptions = ideaPropertiesFile.readLines() | ||
vmOptions.contains("foo=true") |
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.
💅
def vmOptions = ideaPropertiesFile.readLines() | |
vmOptions.contains("foo=true") | |
def ideaProperties = ideaPropertiesFile.readLines() | |
ideaProperties.contains("foo=true") |
@@ -64,6 +71,24 @@ private void logLauncherConfiguration(List<String> commandLine) { | |||
System.out.printf("* Using command line: %s%n%n", String.join(" ", commandLine)); | |||
} | |||
|
|||
private Map<String, String> writeIdeaProperties() { | |||
if (!studioSandbox.getConfigDir().isPresent()) { |
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.
❌ I think we can just create this file also in thestudioSandbox.getJvmArgsDir()
so we can set idea.properties
even if config dir is not present. jvmArgsDir
is just some temp folder that we currently use only for idea.vmoptions
.
🤔 We could then maybe also merge writeAdditionalJvmArgs
and writeIdeaProperties
methods.
💅 Maybe it would be worth also renaming jvmArgsDir
to something else after that
@@ -95,6 +89,7 @@ class ScenarioLoader { | |||
private static final String TOOLING_API = "tooling-api"; | |||
private static final String ANDROID_STUDIO_SYNC = "android-studio-sync"; | |||
private static final String ANDROID_STUDIO_JVM_ARGS = "studio-jvm-args"; | |||
private static final String ANDROID_STUDIO_IDEA_PROPERTIES = "idea-properties"; |
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.
💅 Could we also update an example in README.md and add also a comment what it does.
It might help if we just add example with the configuration we will use (is it gradle.tooling.models.parallel.fetch=true
?). It will then be easy to copy/paste configuration when we will need it :)
.put("STUDIO_PROPERTIES", ideaProperties.toString()) | ||
.put("IDEA_PROPERTIES", ideaProperties.toString()) |
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.
❌ I didn't notice before, but we should pass path to a file here, see this change:
.put("STUDIO_PROPERTIES", ideaProperties.toString()) | |
.put("IDEA_PROPERTIES", ideaProperties.toString()) | |
.put("STUDIO_PROPERTIES", ideaPropertiesFile.toString()) | |
.put("IDEA_PROPERTIES", ideaPropertiesFile.toString()) |
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.
LGTM
Signed-off-by: Sergey Opivalov <[email protected]>
Signed-off-by: Sergey Opivalov <[email protected]>
Signed-off-by: Sergey Opivalov <[email protected]>
Signed-off-by: Sergey Opivalov <[email protected]>
Signed-off-by: Sergey Opivalov <[email protected]>
fb05ec5
to
1ed7b13
Compare
No description provided.