-
Notifications
You must be signed in to change notification settings - Fork 15
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
Refactor CreateDmg and BundleMacosJdk: use Property<..>, migrate to Kotlin #70
base: master
Are you sure you want to change the base?
Conversation
The createDmg test passes locally, however it fails in GitHub Actions :(
|
Hello Vladimir. Thanks for contributing! 🎉 |
@arimer , well, I thought It was summarized in the PR title. I duplicated it to the description as well. |
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.
Thanks a lot for this contribution I have still some open questions and points which are commented on the code.
@@ -1,5 +1,6 @@ | |||
import org.jetbrains.kotlin.gradle.tasks.KotlinCompile | |||
import java.net.URI | |||
import org.gradle.api.tasks.testing.logging.TestExceptionFormat | |||
|
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.
Due the switch to the property API users of this plugins would face problems if they encode a version like 1.4.+ in their project setup.
Therefore I would opt for at least increase the minor verion in L.24.
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.
Maybe @coolya or @sergej-koscejev have an other oppinion.
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.
Frankly speaking, the version is puzzling here, so I need your input here.
Of course, I can bump minor version to 5
import org.gradle.kotlin.dsl.property | ||
import java.io.File | ||
|
||
abstract class BaseBundleMacOsTask( |
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 really like the idea of having a common super class 👍
@@ -0,0 +1,161 @@ | |||
package de.itemis.mps.gradle.test |
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 really like the idea having a minimal RCP test which can be executed.
Could you maybe just drop some lines in the readme about the test idea and the setup?
This would be great and could help others in the future to update/enrich the test if required.
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 puzzled here. Can you please clarify what is not clear?
I'm using a standard approach to use Gradle plugins: org.gradle.testkit.runner.GradleRunner
The test creates Gradle project within a temporary directory and executes a single task there.
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 can't give a good review on this because I don't use these tasks, they run on our build server to produce a DMG for mbeddr RCP and either they work properly or nobody seems to care about DMGs. I also don't understand if there's a particular problem that this PR is supposed to solve.
@vlsi do you intend to maintain these tasks in the future? Have you had problems with these tasks?
Otherwise I'm tending against merging this PR because I'd like to avoid fixing what isn't broken.
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.
…otlin This change bumps the version to 1.5 since it updates APIs for CreateDmg and BundleMacosJdk.
@sergej-koscejev , sorry for the delay. I've rebased the PR and I bumped the version. Do you think it can be merged? |
I can't merge it because the macOS builds fail. @vlsi any idea why? |
I think the build fails since macOS scripts are outdated (see #99) |
So I guess the ways to proceed are:
WDYT? |
I'm fine with updating the macOS scripts. @coolya do you have any opinion? |
I'm fine with updating the files. |
Well, I did check the scripts from JetBrains, and it looks like they are worse than the scripts in mps-gradle-plugin :(( As far as I understand, there's no point in building
So I suggest:
WDYT? |
@vlsi I'm fine with all your suggestions, I just don't want to be the one to do this. Yes, there's no point in building a .dmg without a JDK because the JetBrains JDK contains some improvements that help their IDEs work properly. The IDEs might crash or misbehave on a standard JDK, AFAIK. |
fixes #69
The PR includes the following changes:
Property
API so the tasks support lazily generated inputs. For instance, jdk dependency is downloaded only in case the task is actually executedFile.createTempDir
is replaced withbuild
directory to better conform with Gradle conventionscleanTemporaryDirectories
option for debugging purposes (if set tofalse
it would skip removing the temporary directories used for preparing the output artifact)