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

Feature/cross platform buildscript gradle only #841

Open
wants to merge 10 commits into
base: develop
from

Conversation

Projects
None yet
3 participants
@m0rkeulv
Contributor

m0rkeulv commented May 17, 2018

Here is a new branch with Gradle build script in this branch there is no changes to source code.

Gradle and its plugins uses a lot of conventions and checks for code and resources in default directories. The current build script has to override and/or add to several of these defaults configurations to work.

The one that seems to repeat itself is the lack of resources-directories, everything is mixed in with java code so for most tasks that process or include resources I have had to explicitly tell the what to include from the java source directory, instead if the default that looks for these files in a resources folder.
Another issue is the fact that the src root contains some code (src/icons) some resources (src/META-INF) and another source root! (src/common) there might be a way for Gradle to tell IntelliJ to distinguish between the two source roots but I have not put much effort into solving this as I hope we could change this.

There is a lot of stuff I would suggest to change but for now this should work, I think. Please check it out and see if you see any issues

Suggested "smaller" changes :

  • either move code out of src/common/ (to just src/) or change HaxeIcons.java (src/icons) so that the src/icon folder is not considered as a package. (this is to avoid source trees within source trees)

If you expect or plan different source roots as it was in the past (v14,v15 etc) i would suggest the icon folder change

  • move hxml.bnf from com.intellij.plugins.haxe.hxm to grammar folder where haxe.bnfcurrently is. (keeps things cleaner and easier to find)

I see no reason why one bnf is in the grammar folder and the other hidden in the source tree

  • upgrade/update hxcpp debugger protocol code.
    Run java code generation for JavaProtocol.hx with hxJava 3.20 or later and hxcpp-debugger from haxeFoundation repo protocol_v1.1. this should get rid of the java 8 warnings after you update the plugin specific code.

there is a task that can do this in the current build script but its disabled by default, see generateDebuggerJavaSource task and the generateHxcppDebugger flag

Suggested larger changes :

  • move src , testSrc and testData into a module/directory, i don't have may good arguments for this one, and its probably just my own preferences, but here's a couple of reasons why i what to change it. I find it cleaner when multi-module projects has all its code and resources in modules. it make the gradle script a bit cleaner as as we get to put module specific logic in a gradle file for that module and keep the root gradle for all that is shared between all modules.

  • Rearrange directory/file structure to match modern build systems (Gradle/Maven) and their plugins (java-plugin, gradle-intelliJ-plugin) and help IDEA to detect source roots etc. Doing this is not difficult it does however require a lot of changes, and while existing branches wont have any problem merging into a branch with these changes any new files will have to be moved to the correct directory after merging.

Rearranging directory/file structure to match Gradle/Maven will make the build script cleaner and make the project more familiar to a lot of developers. (ex. on how it might look #833)

Link to Gradle docs (default project layout)
Link to Maven docs (Standard Directory Layouts)

@EricBishton

Thanks for this submission, Mads. I really appreciate all of the effort you've given this. It's also given me a couple of things to think about.

Can you respond to my comments below, and then we'll move toward approval.

<component name="IdProvider" IDEtalkID="B64D6DB6C0FF3B1E51F67FD8EEBC074D" />
<component name="ProjectRootManager" version="2" languageLevel="JDK_1_8" default="false" assert-keyword="true" jdk-15="true" project-jdk-name="IDEA Ultimate" project-jdk-type="IDEA JDK">
<output url="file://$PROJECT_DIR$/out" />
<component name="ProjectRootManager" version="2" languageLevel="JDK_1_8" project-jdk-name="1.8" project-jdk-type="JavaSDK">

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

I'm not sure that we should be making the external 1.8 JDK the default. It should be the JDK shipped with IDEA.

@@ -1,17 +0,0 @@
<component name="ProjectRunConfigurationManager">

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

And, why delete the run configurations?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

When importing a Gradle project IDEA seems to delete a lot of configuration files, i could probably try to restore some of them, if that's wanted. i see the intellij rust plugin mentions the same problems.

Unfortunately during import IDEA may delete .idea/runConfigurations, 
just revert changes in the directory if this happens.

ref: https://github.com/intellij-rust/intellij-rust/blob/master/CONTRIBUTING.md

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

That's one way to handle it, I guess Maybe we can have "master" run configuration templates that can be created/restored as part of a gradle "setup" task? (A doLast{} on import, maybe? Just thinking out loud, here.)

###### Syntax Errors
First thing: You *MUST* build the project to generate the PSI sources. Until you
do, you will have missing classes in many places that will magically disappear when

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

This comment is still pertinent: Users loading the project -- or immediately after running gradle clean -- will see missing (generated) sources until they complete a build.

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 15, 2018

Contributor

I have added some new text to address this problem :)

###### Syntax Errors
Its recommended to build the project first before you start writing code as some parts of the project 
uses generated code and you may experience syntax errors in your code editor, the code will however
build just fine as these sources are generated when the project is built.
 
If you just want to generate the nessesary sources you can run the `generateSources` gradle task.
plugin without ever installing IDEA Ultimate. The ```gradlew``` command will
do everything for you. The first time you run the command for a given IDEA version
it will download IDEA and all dependencies for that version. Any later executions
of this command should execute much faster as it will reused the already downloaded files.

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

So, this may not be the case, when older versions of IDEA get removed from the JetBrains download site. We (Okay, I) need a mechanism to build (and NOT permanently remove on a clean) using versions no longer available for download. So, there needs to be a way for me to specify a local download directory to pick up the installation file (.zip, .exe, .dmg, etc.), and that directory needs to excluded from the cleaning procedures.

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

Clean does not remove downloaded versions of intelliJ
(only the removeIdeaCache task will delete previous versions)
as you can see from the image below i still have older versions even though i target 2018.
image of multiple versions of IDEA

The plugin downloads IntelliJ from Jetbrains repository and currently they have releases all the way back to build 139.1.20 (released back in 2014) so i don't think this will be an issue any time soon.

Jetbrains repository : https://www.jetbrains.com/intellij-repository/releases

I have not tested this but i think unziping a release to the expected folder should work fine, if that's not the case then a few changes in the build script would fix that, but as mentioned above i don't think this will be needed anytime soon.

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

First, that repository only contains the Windows releases. You can't install to Mac or Linux from there (they don't have the Java binaries, for instance). Second, downloads from there are soooooo sloooooooowwwwwww. If we actually used it for CI, it would take an hour to do a build/run on Travis, instead of 5 minutes.

I just want to be sure that when a customer tells me to test issues using 2016.3.3, I can do that and not have a "clean" build remove that version.

Also, I can see that "clean" doesn't delete the idea cache, as you say, so we're OK on that front.

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 11, 2018

Contributor

To me these files looks like they are all-in-one packages, they work just fine on both my mac and my windows computer.
image-of-mac-linux-windows-folders
The Jetbrains JRE is not included by default, from what I can tell it will by default use the same JRE as the one your current IntelliJ instance is using. If you need to use a specific Jetbrains JRE you can provide it in the runIde configuration

runIde {
    jbreVersion 'jbrex8u152b1248.6'
}

(Note that this will be downloaded and stored in Gradles cache and not the project directory)

I have tried to figure out what the main difference between the official openJDK and jetbrains version is, and as far as I can tell it’s just cosmetic stuff like HiDPI support, better font rendering and those kind of things, it should as far as I can tell not affect any plugin logic

When it comes to the speed of the repo, this does not seem to be an issue just look at the Travis build for this branch, all builds completed in approx. 5-6 minutes. From what I can tell files in this repo is also available on a CDN but you might have to manipulate the URL to fetch from the CDN servers (it looks like that’s what the plugin does)

just add /~urlswitch/ after jetbrains.com in the file URL and you get decent download speed.

Ex.
https://www.jetbrains.com/~urlswitch/intellij-repository/releases/com/jetbrains/intellij/idea/ideaIC/181.4445.20/ideaIC-181.4445.20.zip

i get 25-30mb/s when i use these urls :)
image-of-30mbs-donwload-speed

This comment has been minimized.

@EricBishton

EricBishton Jun 11, 2018

Member

/~urlswitch/ is something that I did not know about. Since the speed seems decent, then we'll go with it.

I don't remember the packages being multi-target out there. I had even asked a question of why only Windows targets were out in the repository. The JetBrains folk's anwer was, "Why do you need other targets for CI?" That was back in the 14.1 days, so things might have changed since then. Or... I just missed that they were really multi-target.

## Build troubleshooting
Once in a while you might experience that your build/run/debug/test task fails and the error reported seems to be related to gradle.
This can sometimes happen when you have multiple gradle targets open at the same time. Check your debug and run panels and try to close
any extra tabs that might be open.

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

Can we notice this and refuse to start until another build, test, etc. has finished?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

If i recall correctly this was/is a file lock issue. Not sure if this issue is caused by gradle, IntelliJ or just Windows being stupid, but its gradle who complains about a file being locked when you try to run a second target. i don't know if this issue occurs on Mac or Linux, i could probably test this at some point.

@@ -0,0 +1,326 @@
/*
* Copyright 2018-2018 m0rkeulv

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

You can use this name for copyright if you want, but I wouldn't suggest it. If you ever want to enforce the copyright or change the license agreement, a user-name will be hard to use for legal purposes.

println 'to specify build target use -PtargetVersion=<version>'
println 'Ex:'
println 'gradlew buildPlugin -PtargetVersion=2017.2.2'
println 'If your running "gradle clean" just ignore this warning'

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

We can't test for this at line 25 and not display the warning in the first place?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

i am not sure if i follow you on this one?
Line 25 checks if you have provided a version or not. this warning only occurs if the version argument is missing, the only other option would be to stop the build and enforce the version argument.

Line 25 : if (!project.hasProperty('targetVersion')) {

the targetVersion comes from commandline

# ex. : 
gradlew clean build -PtargetVersion=2016.3

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

I mean something like

Line 25: if (!project.hasProperty('targetVersion') && !target.is("clean")) {

That's probably not proper gradle syntax, and I don't know how to check the current target, but that's the gist of what I am thinking.

BTW, if we still need line 32, make it 'If you're running "gradle clean," just ignore this warning.'

args = ['grammar-kit.jar', "${generatedSrcDir}", "${grammarHaxe}"]
inputs.file "${grammarHaxe}"
outputs.upToDateWhen { file("${generatedSrcDir}/com/intellij/plugins/haxe/lang").exists() }

This comment has been minimized.

@EricBishton

EricBishton Jun 8, 2018

Member

... and input are not newer. Or, is that implied by inputs.file?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

My understanding of the input and output configuration is that the provided files are hashed and compared by the task, so if there are any changes to the file the task is no longer up-to-date.

https://discuss.gradle.org/t/where-how-does-gradle-store-up-to-date-checks/12240

Gradle hashes file content for both inputs and outputs.
This means that a change to the output (or in this case a stale out put)
should cause the task to be out of date.

This comment has been minimized.

@EricBishton
args = ['grammar-kit.jar', "${generatedSrcDir}", "${grammarHxml}"]
inputs.file "${grammarHxml}"
outputs.upToDateWhen { file("${generatedSrcDir}/com/intellij/plugins/haxe/hxml").exists() }

This comment has been minimized.

@EricBishton

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 9, 2018

Contributor

see comment above

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 8, 2018

  • either move code out of src/common/ (to just src/) or change HaxeIcons.java (src/icons) so that the src/icon folder is not considered as a package. (this is to avoid source trees within source trees)

If you expect or plan different source roots as it was in the past (v14,v15 etc) i would suggest the icon folder change

I'm OK with moving icons to a separate directory structure. However, 18.2 is going to change all of the icons to a grey versions, so we're going to need icon sets that are particular to specific versions. :/ And, maybe even a checkbox to use "colored" icons if selected. In that case, though, we'll have to load them at run-time instead of by version. Let's work on this in a follow-on PR.

  • move hxml.bnf from com.intellij.plugins.haxe.hxm to grammar folder where haxe.bnf currently is. (keeps things cleaner and easier to find)

I see no reason why one bnf is in the grammar folder and the other hidden in the source tree

Totally agree on this one. I'm also in favor of moving the flex descriptions to a similar directory. The generated output from both of them (parser and lexers) should end up in the /generated tree.

  • upgrade/update hxcpp debugger protocol code.
    Run java code generation for JavaProtocol.hx with hxJava 3.20 or later and hxcpp-debugger from haxeFoundation repo protocol_v1.1. this should get rid of the java 8 warnings after you update the plugin specific code.

there is a task that can do this in the current build script but its disabled by default, see generateDebuggerJavaSource task and the generateHxcppDebugger flag

We really need to rewrite the debugger to use the JSON-based protocol and drop support for this old one. Of course, that protocol isn't quite finished, so there's a bit of work to do there. At any rate the current protocols use data structures that are terrible to unwind when you have to debug the debugger.

For your larger suggestions, I'm not on board with those yet. We may get there, but I'm not ready to make the larger source rearrangements yet.

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 8, 2018

Oh, yeah. We should also add the capability to build and deploy the [plugin] EAP version that Ilya added a couple of months ago.

Here is a version from one of our team members:
For these to work with IDEA you will have to tell IDEA to delegate te build job to Gradle

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

te -> the. Add a period at the end of the sentence.

#### IDEA builds
The Gradle Project panel might be a bit overwhelming for those who are not used to Gradle

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

Missing period.

This comment has been minimized.

@EricBishton

EricBishton Jun 9, 2018

Member

There are a number of missing periods in this document. A quick scan and cleanup is warranted.

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 9, 2018

It occurred to me as I'm reading through your comments again that we're using the complete IDEA version as the ideaVersion variable everywhere. We don't want to be creating jars with the name '2018.1.4', for example. The name should be truncated at the minor version number unless there's a reason to use the path version (which only should happen if JetBrains messes up and introduces a breaking change in a patch).

}
task checkForMissingRunConfigurations() {

This comment has been minimized.

@EricBishton

EricBishton Jun 11, 2018

Member

When does this get called? Or is it supposed to be a menu-only thing?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 13, 2018

Contributor

Gradle has different phases; the logic in this task is in the configuration phase and will be executed whenever Gradle “evaluate/reads” this task. It was the easiest way to get logic executed and runConfigurations created for intelliJ without having to get the user to build or run any tasks.

Placing it in the Initialization phase could also be an option
https://docs.gradle.org/current/userguide/build_lifecycle.html

createGradleRunConfiguration('Run Tests 2018', 'clean test', "-PtargetVersion=$latest2018Version", '')
createGradleRunConfiguration('Run Tests 2017', 'clean test', "-PtargetVersion=$latest2017Version", '')
createGradleRunConfiguration('Run Tests 2016', 'clean test', "-PtargetVersion=$latest2016Version", '')

This comment has been minimized.

@EricBishton

EricBishton Jun 11, 2018

Member

I'm not sure about needing versioned run targets. This will run the currently built project (built with whatever IDEA you are running) against a specific version of IDEA, right? So, running the 2017.2 target from 2018.1 won't work because of the incompatibilities, correct? Or am I getting that wrong and the 2017.2 version of IDEA will be used to build prior to the tests being run, so that the environments will be consistent?

This comment has been minimized.

@m0rkeulv

m0rkeulv Jun 11, 2018

Contributor

there should not be any incompatibilities as far as i know (2016, 2017, 2018 all works in my 2017.3 install, but if you find its cluttering the project i could remove them).

The build command will use the javac2 compiler, sources and libs from the version of intelliJ that the configuration specify (it will download it if its missing), so it should be pretty close if not identical to what you would get if you compiled it with an installed version of intelliJ with the same version number.

if you build using the --debug flag you will see that version specific files that are resolved for the build.

2016 runConfiguration

Build operation 'Resolve lib/idea.jar (com.jetbrains:ideaIU:2016.3.8)' completed
Build operation 'Resolve lib/idea_rt.jar (com.jetbrains:ideaIU:2016.3.8)' completed
Build operation 'Resolve lib/ideax.jar (com.jetbrains:ideaIU:2016.3.8)' completed
Build operation 'Resolve lib/eddsa-0.2.0.jar (com.jetbrains:ideaIU:2016.3.8)' started
....
Build operation 'Resolve lib/javac2.jar (com.jetbrains:ideaIU:2016.3.8)' started 

2017 runConfiguration

Resolve lib/jasper21_rt.jar (com.jetbrains:ideaIU:2017.3)' started
Resolve lib/jdkAnnotations.jar (com.jetbrains:ideaIU:2017.3)' started
Resolve lib/jasper2_rt.jar (com.jetbrains:ideaIU:2017.3)' started
Resolve lib/idea.jar (com.jetbrains:ideaIU:2017.3)' completed
....
'Resolve lib/javac2.jar (com.jetbrains:ideaIU:2017.3)' started`

The plugin also creates a sandbox for the version you provide and starts and instance of that version with the plugin installed, so the only incompatibility i can think of is older versions not supporting java 8 or something like that.

This comment has been minimized.

@EricBishton

EricBishton Jun 11, 2018

Member

OK, then. Great.

createGradleRunConfiguration('Run IDE 2016',
'runIde',
"-PtargetVersion=$latest2016Version",
'-Xmx1024m -Xms512m -XX:MaxPermSize=256m -ea -DHAXELIB_LIST_PATH_SUPPORTED=1')

This comment has been minimized.

@EricBishton

EricBishton Jun 11, 2018

Member

For these, it would be really nice to add -Didea.is.internal=true which turns on the PSI viewer in the Tools menu.

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 16, 2018

By my reading, the only thing left undone is to add the functionality to create and deploy the nightly build that Ilya (mayakwd) added to the ant build.

@m0rkeulv

This comment has been minimized.

Contributor

m0rkeulv commented Jun 17, 2018

Not quite sure what I am missing for the nighty builds or how to test them.
I have added the ... if branch = develop AND type = push ... job configuration to the Travis.yml

And I use the parameter from DEV_BUILD=.dev.${TRAVIS_COMMIT::7}
as part in my filtering in build.gradle line 301 if the parameter is set.

properties.setProperty('plugin.dev.version', project.hasProperty('devBuild') ? "${project.devBuild}" : '')

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jun 18, 2018

Not quite sure what I am missing for the nighty builds or how to test them.

@mayakwd Have any ideas on this?

@m0rkeulv For my part, I know that we publish the nightlies to JetBrains repo via their RESTful API. We name the jar file with the commit number and publish on the EAP channel. The publication uses Ilya's (mayakwd's) credentials. I don't know how Ilya tested it.

@mayakwd

This comment has been minimized.

Collaborator

mayakwd commented Jun 19, 2018

As I see - everything configured properly and should work. Anyway if it's not - we could fix it later.

@mayakwd mayakwd self-requested a review Jun 19, 2018

@mayakwd

Good job!

@EricBishton

This comment has been minimized.

Member

EricBishton commented Jul 30, 2018

Hi Mads, I'm sitting on this for the moment, just because I'm hip deep in a few other tasks and this may (will??) break my normal work flow. I really appreciate and want this PR. I'll pull it as soon as I'm done with my pieces that are in-process. Thanks for your patience. -Eric

@m0rkeulv

This comment has been minimized.

Contributor

m0rkeulv commented Jul 31, 2018

no problem :)

perhaps you should wait until the next release is done, that way you have a nice working version to revert to in case there should be any issues. i also don't want it to be causing any delay for the Haxe 4 support (i am currently using the develop branch just to get the haxe 4 features)

@EricBishton

This comment has been minimized.

Member

EricBishton commented Aug 1, 2018

Good idea. I just resolved the .travis.yml conflict with the "New project content" change that @RealyUniqueName submitted.

@EricBishton

This comment has been minimized.

Member

EricBishton commented Aug 1, 2018

Harrumph! All of the mac builds are failing. Somehow we picked up Java10 for the mac builds, instead of Java8 which we picked up two months ago. Must be a travis-ci configuration change. I'll get back to this...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment