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

Migration to MPS 2020.1 #73

Merged
merged 7 commits into from
Oct 20, 2020
Merged

Migration to MPS 2020.1 #73

merged 7 commits into from
Oct 20, 2020

Conversation

wsafonov
Copy link
Member

This PR migrates mps-gradle-plugin to MPS 2020.1.3.

There were no changes related to the MPS start-up classpath configuration for the latest MPS version, which means that the gradle plugin remains compatible with previous MPS version and we don't need to increase the plugin version.

- write test build classpath with platform-independent separators to
avoid the issue with `\` path separators on windows causing unescaped
string exception when running the test build
- mark `overrides` property as InputFile and use simple assignment in
task instead of the setter
- ScriptBuilder constructor requires FacetRegistry parameter
- use TargetConceptChecker2 instead of deprecated TargetConceptChecker
- provide now required ComponentHost parameter in ModelPropertiesChecker
@@ -12,7 +12,7 @@ class GenerateLibrariesXml extends DefaultTask {
@InputFile
File defaults

@Optional
@Optional @InputFile
Copy link
Contributor

Choose a reason for hiding this comment

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

I think we can not set this to input file since it will cause an exception if the file doesn't exists. That was the reason why in the past we removed this annotation and introduced the setter method which adds it to the dependencies when file actually exists.

See the tests that you changed here src/test/kotlin/GenerateProjectLibrariesXml.kt and removed the one that tests with a none existing overrides file.

Copy link
Contributor

Choose a reason for hiding this comment

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

Also removing that set method is a breaking API which we can't just do as an MPS upgrade and I would like to avoid.

Copy link

Choose a reason for hiding this comment

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

The workaround is to use @InputFiles: gradle/gradle#2016

However, I agree it would be better to decouple the change from MPS upgrade

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's strange, cause the official documentation states that the task validation should not fail in combination with @Optional annotation on @InputFile property if the file doesn't exist: https://docs.gradle.org/current/userguide/more_about_tasks.html#sec:task_input_output_validation But you are right - currently the test for non-existing overrides file would fail.

The reason why I actually tried to refactor this is because task validation in validatePlugins task (which we are currently not using in the official build on the build server) failed with a warning due to a property w/o any input or output annotations. Simply using @Input annotation doesn't help (still produces a warning, cause @Input on Files is deprecated and will be removed in Gradle 7). So @InputFile was the obvious choice.

It feels strange from the user point of view that overrides unlike other properties requires a setter-style initialization, so I would rather like to refactor this as a breaking change with increased minor number. But given the fact that Gradle currently doesn't really properly support the case with non-existing files, I don't know if it's a good idea. Maybe give a try to the @InputFiles workaround if this would behave like a normal property from the user point of view.

So for now I reverted those changes and just set @Input annotation as a reminder, extended with some documentation comments.

Copy link
Member

Choose a reason for hiding this comment

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

As a reminder, let's consider dropping this task altogether: #62

@vlsi vlsi mentioned this pull request Jul 23, 2020
# Conflicts:
#	build.gradle.kts
#	execute-generators/gradle/dependency-locks/compileClasspath.lockfile
#	modelcheck/gradle/dependency-locks/compileClasspath.lockfile
#	project-loader/gradle/dependency-locks/compileClasspath.lockfile
@wsafonov wsafonov requested a review from coolya October 19, 2020 12:28
@wsafonov wsafonov merged commit 8e937d2 into master Oct 20, 2020
@wsafonov wsafonov deleted the feature/mps20201 branch October 20, 2020 15:38
@abstraktor
Copy link
Contributor

Thanks! A client asked us about this just today!

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.

5 participants