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

simplify sample usage #623

Open
wants to merge 8 commits into
base: master
Choose a base branch
from

Conversation

dima-avdeev-jb
Copy link
Contributor

@dima-avdeev-jb dima-avdeev-jb commented Nov 27, 2022

Users need an easy way to launch our sample (https://stackoverflow.com/questions/74393070/is-skiko-right-now-only-available-for-jvm-awt).

  • Set a fixed skiko version in sample.
  • Add options to use composite build in several samples.
  • Describe how to run samples with different dependencies.
  • Fix run configurations.

After this PR, users may run samples as is, without additional steps.
In the future, we should update a skiko version in samples/*/gradle.properties.

DEVELOPMENT.md Outdated
```

### samples/SkiaJsSample with mavenLocal
If page in browser will be empty, try to reload it.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
If page in browser will be empty, try to reload it.
If the page in the browser is empty, try to reload it.

Anyway, its seems like a bug, and better to not mention it in Readme? Because if the page is empty, reloading it usually is the first thing to try.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, I think we can remove this comment. It's obviously.
Done

DEVELOPMENT.md Outdated
```

### samples/SkiaJsSample with skikoCompositeBuild=true
It means: includeBuild("skiko") with dependency substitution. It compiles faster.
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
It means: includeBuild("skiko") with dependency substitution. It compiles faster.
When you add `-DskikoCompositeBuild=true`, Skiko from [sources](https://github.com/JetBrains/skiko/tree/master/skiko) is used (without this property the maven artifact is used). Every time you change sources, they are automatically applied when you run `jsBrowserDevelopmentRun`.

includeBuild usually doesn't tell what this property does, even if people are familiar with the Compose Gradle build feature. Also, it is used not to build project faster, but to use skiko from sources, not as prepublished maven artifact.

It also means, that we don't need to call publishSkikoWasmRuntimePublicationToMavenLocal and add -Pskiko.version=0.0.0-SNAPSHOT.

But see my other suggestion, if we do that way (use Composite build by default), we don't need this section at all.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We still need to call publishSkikoWasmRuntimePublicationToMavenLocal.
It's a different jar, packaged by tricky script.

@@ -8,7 +8,7 @@ pluginManagement {
}
rootProject.name = "SkiaMultiplatformSample"

if (System.getenv("SKIKO_COMPOSITE_BUILD") == "1") {
if (System.getenv("SKIKO_COMPOSITE_BUILD") == "1" || System.getProperty("skikoCompositeBuild") == "true") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Let's add this section to SkiaAwtSample too.

@@ -10,3 +10,10 @@ pluginManagement {
}
rootProject.name = "SkiaJsSample"

if (System.getenv("SKIKO_COMPOSITE_BUILD") == "1" || System.getProperty("skikoCompositeBuild") == "true") {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Suggested change
if (System.getenv("SKIKO_COMPOSITE_BUILD") == "1" || System.getProperty("skikoCompositeBuild") == "true") {
// This section is needed to use Skiko from sources instead as a maven artifact. If you use Skiko in your own project outside of this repository, you don't this section.
if (File("../../skiko").exists()) {

I assume, when we develop Skiko - there is no point to use Skiko from maven repo. And if users just copy the example, it will work too.

This change will simplify the whole process:

  • we can just run the example with a simple Gradle command without additional properties (./gradlew run). It will work as in our use case, and in user's use case (when they copy the example)
  • we can open the example in IDEA without adding a global SKIKO_COMPOSITE_BUILD (I just checked the AWT sample, highlighting and debugging work)

If we do this change, we can (and should) simplify the Readme and the CI file - there will be no need to call publishAllSkikoJvmRuntimeToMavenLocal and define skiko.version.

Also, because of this, we can remove skiko.version property completely, and just use 0.7.41 constant instead of it.

(we can discuss this online, to sync on the final decision)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a good suggestion. But we have problems with this case.

includeBuild("skiko") provides only org.jetbrains.skiko:skiko:1.2.3 artifact.

When we run task publishAllSkikoJvmRuntimeToMavenLocal, under the hood this task downloads and packs skia inside jar and saves to mavenLocal several additional dependencies org.jetbrains.skiko:skiko-awt-runtime-macos-arm64:1.2.3, org.jetbrains.skiko:skiko-awt-runtime-macos-x64:0.0.0, org.jetbrains.skiko:skiko-android-runtime-arm64:1.2.3 and others.

Also, we have task publishSkikoWasmRuntimePublicationToMavenLocal to save org.jetbrains.skiko:skiko-js-wasm-runtime:1.2.3

I think we also can solve this problem, but I don't know how...
For this case, we need to rewrite publishing tasks and also provide modules instead of jars:

allJvmRuntimeJars.forEach { entry ->

@@ -1,3 +1,7 @@
rootProject.name = "skiko-all"
Copy link
Collaborator

Choose a reason for hiding this comment

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

This file (the root setting.gradle.kts was added in the old times, when we had only the AWT implementation, and needed a way to open the project in IDEA as whole. Let's remove it to avoid confusion. Now we have a better way (opening individual examples)

But after removing, we should change the CI file (call gradlew not from the root folder, but from the skiko folder.

DEVELOPMENT.md Outdated Show resolved Hide resolved
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.

None yet

3 participants