-
Notifications
You must be signed in to change notification settings - Fork 122
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
base: master
Are you sure you want to change the base?
simplify sample usage #623
Conversation
DEVELOPMENT.md
Outdated
``` | ||
|
||
### samples/SkiaJsSample with mavenLocal | ||
If page in browser will be empty, try to reload 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.
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.
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.
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. |
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.
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.
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.
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") { |
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.
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") { |
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.
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)
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.
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:
Line 1232 in 90fb598
allJvmRuntimeJars.forEach { entry -> |
@@ -1,3 +1,7 @@ | |||
rootProject.name = "skiko-all" |
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.
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.
Users need an easy way to launch our sample (https://stackoverflow.com/questions/74393070/is-skiko-right-now-only-available-for-jvm-awt).
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.