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

RFC: Infrastructure to execute code with a project #25

Merged
merged 15 commits into from
Nov 22, 2018
Merged

Conversation

coolya
Copy link
Contributor

@coolya coolya commented Jul 30, 2018

This PR is not meant to be merged yet. I opened it to get feedback as early as possible and to discuss potential further work.

Infrastructure to execute code with a project

As we are more and more often required to execute certain tasks during the build the IdeaEnvironment becomes the go to solution. We also have some further plans to add new functionality to this plugin that requires access to a loaded MPS plugin (#21, #22). This PR aims to provide a common infrastructure to make these functionality as easy to use as possible. To do so the common bits of the execute-generators subproject were extracted.

Java API

The Java API is a single function executeWithProject it has been written in Kotlin but is perfectly callable from normal java code as a static function but is more idiomatic Kotlin and java. It might feel strange to java programmers to work with such API especially as it's more a functional style than normal OO.

The function takes all arguments required to setup the IdeaEnvironment plus a function that gets the project once it's loaded. The function can produce a value which is returned to the caller. The executeWithProject function also takes care of the complete lifecycle of the project and the IdeaEnvironment.

Discussion

  • Is the idiomatic Kollin API good enough for java developers or do we need a more OO like facet?
  • Are the use cases were caching the the IdeaEnvironment is essential over several project opening?
  • Error logging/handling is currently mostly missing

Command line API

In addition to the actual Java API I tried to standardise a command line API. As most of our current use cases spawn a new JVM rather then loading all of MPS into the same JVM as gradle we often need to pass information around via command line arguments. To avoid reinventing the wheel and having multiple incompatible implementations floating around I created a extensible command line argument definition. It takes all required parameters and allows passing them to a overload of the executeWithProject directly.

Application specific arguments can be added by extending the Args class.

Discussion

  • Do we want to standardise this interface?
  • The reuse is limited to Kotlin as the argument parser isn't really usable from oder JVM languages.

Reuse

The project-loader is reusable beyond the scope of this project. We could point others who want to do something with a headless MPS to this plugin and it's command line interface. That's why I would suggest publishing the project-loader as a separate maven publication. This comes with the downside that we will have to be more carefully of introducing breaking changes as we will not have all of the users under our control.

Open tasks:

  • update GenerateMpsProjectPlugin to use new command line API.

Moved all the logic required to start up a idea environment into the
`project-loader` subproject. The subproject contains all code required
to load plugins and open a project. The loader also takes care of the
complete lifecycle of the idea application and the project. Users of The
loader pass a lambda/function that takes the project as an argument. In
case of any errors during the execution of the lambda or when the lambda
is executed successfully the loader tries to gracefully shut down the
idea environment.

In addition to that the loader specifies a basic command line interface
for potential users of the loader.

The `execute-generators` subproject is the first user of the new
`project-loader`. Since it required changes to the command line
interface this is a breaking change.
@coolya coolya changed the title RFC/WIP: Infrastructure to execute code with a project WIP/RFC: Infrastructure to execute code with a project Jul 30, 2018
@coolya coolya changed the title WIP/RFC: Infrastructure to execute code with a project RFC: Infrastructure to execute code with a project Aug 4, 2018
@coolya
Copy link
Contributor Author

coolya commented Sep 7, 2018

I would appreciate any feedback on this PR.

@sergej-koscejev
Copy link
Member

I'll review this on Monday.

Copy link
Member

@sergej-koscejev sergej-koscejev left a comment

Choose a reason for hiding this comment

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

The biggest problem is the macros vs macro typo :) I leave the rest up to you to decide.

{ toPlugin(this) }

val macros by parser.adding("--macros",
help = "macro to define. The format is --macro=<name>:<value>")
Copy link
Member

Choose a reason for hiding this comment

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

so is this --macros or --macro?

* @param buildNumber optional build number that is used to determine if the plugins are compatible. Only guaranteed to
* work with MPS 2018.2+ but might work in earlier versions as well.
*/
fun <T> executeWithProject(project: File,
Copy link
Member

Choose a reason for hiding this comment

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

How about bundling the environment options into a class? I'm on the fence about this myself but maybe the class will be a natural place to add methods to in the future.

Copy link
Member

Choose a reason for hiding this comment

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

Ah ok, I see below that the Args class is playing that role. It's probably enough then.


if (pluginLocation != null) {
logger.info("overriding plugin location with: ${pluginLocation.absolutePath}")
System.setProperty(PROPERTY_PLUGINS_PATH, pluginLocation.absolutePath)
Copy link
Member

Choose a reason for hiding this comment

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

Should we set the property to the old value when we're finished? To clean up after ourselves.


if (buildNumber != null) {
logger.info("setting build number to \"$buildNumber\"")
System.setProperty(PROPERTY_PLUGINS_COMPATIBLE_BUILD, buildNumber)
Copy link
Member

Choose a reason for hiding this comment

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

Ditto.

@sergej-koscejev
Copy link
Member

I would like to have this functionality accessible from Maven so I'm looking into this now.

A few years ago I wrote mps-maven-plugin where I used a similar approach but I used the GeneratorWorker and Script classes from mps-tool.jar to start the generator.

This would allow us to save some code and it seems to be a more "official" entry point since GeneratorWorker/Script are used by the generate Ant task. What do you think about that approach?

@coolya
Copy link
Contributor Author

coolya commented Oct 22, 2018

@sergej-koscejev Sorry for the very late reply. If I recall the details correctly, I think the major reason was that the MPS-generate task does require you to give it a calculated make script with all the libraries and also to calculate the chunks manually. Which the LangaugeWorkbenchMakeService does for you.

When using the tests we need to know the version ot the plugin we are
testing against statically, because there is no code can run before
the plugin block of a build script is evaluated. I added parameter to
force a snapshot version that we can then use in the tests.
@coolya
Copy link
Contributor Author

coolya commented Oct 23, 2018

While manually testing the build scripts I found a bug in the argument parser and fixed it. I wanted to setup automated tests for the gradle scripts but this turned out to be harder than expected.

I needed to update to gradle 4.10 to get support for snapshot plugin versions as a start.

The next problem is testing against the plugin version that was just build. I tried as suggested in the documentation by using a composite build in the test project that includes the plugin build, but I was unable to get gradle to find the plugin with this setup.

The only workaround I found is that I publish a snapshot version of the plugin to maven local and the use maven local as a plugin repository in the tests. This seems to work but will required us to do some changes in the CI to have the tests execute correctly. I thought about using gradles testkit to implement the tests but then getting the required mps projects into the build location wasn't really straight forward. So I opted for adding independent gradle projects to the repo that use maven local to get the plugin.

To test this manually you need to do the following:

  • In the root project execute ./gradlew publishToMavenLocal this will put the gradle plugin (which is mostly a pom file) and the other artefacts into maven local
  • in test/generate-* run ./gradlew generate this will pickup the plugin from maven local eventually succeed generating the project.

I added two smoke tests, one that generates a simple java class and a second one that generate a build.xml file from the mps build project.

@coolya
Copy link
Contributor Author

coolya commented Nov 4, 2018

Form my perspective this is ready to be merged. See my above comments to see what I changed from the last review. I would also suggest that we now bump the version number to 1.1 which we forgot to do last time.

If anyone used the command line generation with the current version these changes would break the command line interface to the spawned java subprocess. So just to be sure I bumped the version to 1.1.

@coolya coolya mentioned this pull request Nov 4, 2018
@coolya coolya merged commit d0ba845 into master Nov 22, 2018
@coolya coolya deleted the project-loader branch November 22, 2018 16:45
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.

2 participants