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

Build with Gradle #179

Closed
wants to merge 26 commits into from
Closed

Build with Gradle #179

wants to merge 26 commits into from

Conversation

eshepelyuk
Copy link
Contributor

MIgration of build system to Gradle.
Some unit tests were modified, 1 unused was deleted to make Gradle adoption easier.
fixes #178 fixes #168

Closes #57

Done

  • usage of Gradle wrapper, no local Gradle installation needed
  • compile sources and tests with Gradle
  • run unit tests and benchmark testing
  • publishing to Maven compatible repositories with Gradle
  • legacy Ant and Maven scripts are still working
  • support of quick import from IDE, especially IntelliJ IDEA due to Gradle project nature

Postponed for further review, until PR approval

  • code structure reorganization
  • deletion of legacy scripts
  • README updates
  • usage of default build directory, now custom folder buildGradle is used to prevent conflicts with Ant

Not planned

  • creating ZIP artifacts like one currently produced by Ant. It seems no need to have one
  • release management, i.e. automatic version update, creating GIT tags etc
  • EMMA test code coverage

@eshepelyuk
Copy link
Contributor Author

Hello @gbrail, could you review the PR and provide your feedback.
Thnx in advance.

@eshepelyuk eshepelyuk changed the title Build with Gradle Resolves #178 Build with Gradle Apr 26, 2015
@gbrail
Copy link
Collaborator

gbrail commented Apr 29, 2015

Working on testing this -- I hope to get more time next week.

Can you try and figure out why I have to put a bunch of Maven stuff in my global "gradle.properties," including a repo URL, username, and password? I think that if we want everybody to use this (and we do) then they shouldn't have to configure that stuff. I can give details if you want...

@eshepelyuk
Copy link
Contributor Author

Hello,
That's the thing I've missed to describe. You're right - there's no need to put those properties into gradle.properties of root folder.

Just create a file ${HOME}/.gradle/gradle.properties and put repository URLs and credentials properties there. This is how it is intended to work.

I'll update README.md with Gradle related changes soon.

@gbrail
Copy link
Collaborator

gbrail commented Apr 29, 2015

Is there a way to write the Gradle files so that those properties are
optional? Most people building this product won't have those.

On Wed, Apr 29, 2015 at 9:19 AM, Evgeny Shepelyuk notifications@github.com
wrote:

Hello,
That's the thing I've missed to describe. You're right - there's no need
to put those properties into gradle.properties of root folder.

Just create a file ${HOME}/.gradle/gradle.properties and put repository
URLs and credentials properties there. This is how it is intended to work.

I'll update README.md with Gradle related changes soon.


Reply to this email directly or view it on GitHub
#179 (comment).

greg brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail

@eshepelyuk
Copy link
Contributor Author

Pushed the fix, plz update.

@gbrail
Copy link
Collaborator

gbrail commented May 1, 2015

Making good progress...

I think that there are two more things we should do:

First, of all the "compile" ant task (on line 43 of build.xml) copies all the source into the build directory before compiling, and uses an ant "filter" task to replace the macro @IMPLEMENTATION-VERSION

@gbrail
Copy link
Collaborator

gbrail commented May 1, 2015

...

First, of all the "compile" ant task (on line 43 of build.xml) copies all the source into the build directory before compiling, and uses an ant "filter" task to replace the macro @IMPLEMENTATION-VERSION@ with the version from build.properties. Without doing this macro replacement, then you get this:

$ java -jar buildGradle/libs/rhino-1.7.7-SNAPSHOT.jar
@IMPLEMENTATION.VERSION@
js>

Instead of:

Rhino 1.7.7 2015 04 30
js>

Gradle can do this -- or we can find a better way since it's just one property in one file (it could look it up from the JAR manifest I think). But either way we should do it.

Second, we have always distributed the result of "ant dist" on the Mozilla web site, so if we want to remove ant and put in gradle instead, then we should include the ZIP file that results from this command.

Also, it'd be nice if there was an equivalent of "ant junit-benchmarks". But that can come later.

If you can fix the IMPLEMENTATION.VERSION easily, then I think we should merge this. Then we can see about removing the ant stuff.

@eshepelyuk
Copy link
Contributor Author

  1. As I've mentioned in PR description, we can run benchmark tests with Gradle. Just try to run gradlew testBenchmark.
  2. IMPLEMENTATION_VERSION will be fixed.
  3. About zip archive, what is the purpose of distributing it ? Should it be published to Maven repo as artifact ? What should be the content of that file ? I just see no good reasons now to copy old ant behaviour since we could just zip jars produced for publishing to maven. Please give me some clues about this archive purpose.

P.S. I will be able to provide fixes only starting from Monday.

@gbrail
Copy link
Collaborator

gbrail commented May 1, 2015

OK -- no worries. I can do a few things to fix it. I don't like the way that we filter ALL the source anyway so I'm going to change it in ant and then in Gradle. No need to do anything until you hear from me.

@eshepelyuk
Copy link
Contributor Author

Well, maybe if you can wait couple if days I will push those fixes. I am planning to do in in Java and remove it from build script responsibilities.

@gbrail
Copy link
Collaborator

gbrail commented May 1, 2015

Of course, and we can take our time to get this right.

I made a small change and pushed it to a build called "greg-gradle." If you (having a lot more Gradle experience than I) think that it solves the immediate problem, then I think we can push this to master and then iterate from there.

Can you take a look:

https://github.com/mozilla/rhino/blob/greg-gradle/build.gradle

Basically, I added a task to copy the contents of "src/org/mozilla/javascript/resources" into the build directory, with filtering -- without that, the message property bundle was missing.

WIth this change, other than the "dist.zip" I think that we have Gradle doing everything that Ant did.

@eshepelyuk
Copy link
Contributor Author

Looks good, not sure we need afterEvaluate closure.

@eshepelyuk
Copy link
Contributor Author

Pushed fix to obtain Rhino display version from MANIFEST.MF and updated README to reflect Gradle related changes.
So, as soon as this PR will be merged, I will create new one with final cleanups, i.e. removing ant and maven scriots, restructuring source folders and using default build folder.

@eshepelyuk
Copy link
Contributor Author

@gbrail have you ever expirienced failures in MozillaSuiteTest.
I see some tests are failing from time to time either running with Ant or Gradle.
But the failing tests are non permanent. They are failing just from time to time.

@gbrail
Copy link
Collaborator

gbrail commented May 4, 2015

Yes, there are a few tests that are timing-related. For instance, there is
one now that fails because it expects a certain performance characteristic
from global variables. It is not consistent. I'm not sure what to do about
these but we do have a few right now.

Interestingly, some of them perform differently when run via ant versus
gradle, which is especially strange.

On Mon, May 4, 2015 at 12:01 PM, Evgeny Shepelyuk notifications@github.com
wrote:

@gbrail https://github.com/gbrail have you ever expirienced failures in
MozillaSuiteTest.
I see some tests are failing from time to time either running with Ant or
Gradle.
But the failing tests are non permanent. They are failing just from time
to time.


Reply to this email directly or view it on GitHub
#179 (comment).

greg brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail

@eshepelyuk
Copy link
Contributor Author

  1. Gradle uses own tools for running JUnit and I've tried to emulate all the options from Ant build, we can't be confident that runners behave the same way.
  2. Gradle uses some daemon process and caches when running build, so it could affect build
  3. And last but not the least - Gradle should consume more PC resources on its own and it could affect GC cycles, thus affecting those tests.

@gbrail
Copy link
Collaborator

gbrail commented May 4, 2015

I am, however, seeing a null pointer exception in the re-factored
"ComplianceTest" that only happens when running on the CI machine, and not
locally:

http://ci.apigee.io/job/Mozilla%20Rhino%20Custom%20Branch/25/testReport/junit/org.mozilla.javascript.tests.commonjs.module/ComplianceTest/initializationError/

I'm not sure what's going on unless the directory that it's looking up in
line 34 doesn't exist or is not a directory.

On Mon, May 4, 2015 at 12:01 PM, Evgeny Shepelyuk notifications@github.com
wrote:

@gbrail https://github.com/gbrail have you ever expirienced failures in
MozillaSuiteTest.
I see some tests are failing from time to time either running with Ant or
Gradle.
But the failing tests are non permanent. They are failing just from time
to time.


Reply to this email directly or view it on GitHub
#179 (comment).

greg brail | apigee https://apigee.com/ | twitter @gbrail
http://twitter.com/gbrail

@eshepelyuk
Copy link
Contributor Author

Could you check if files really exists in job's build folder ! I can't access it on my own.
Btw, the job still executes Ant, not Gradle. Is it intentional ?

@eshepelyuk
Copy link
Contributor Author

I gave up struggling to create Pull Request to eshepelyuk-gradle branch with the possible fix.
So could you change CompianceTest line 34

from

final File[] files = new File(ComplianceTest.class.getResource("1.0").getPath()).listFiles();

to

final File[] files = new File("testsrc/org/mozilla/javascript/tests/commonjs/module/1.0").listFiles();

@eshepelyuk
Copy link
Contributor Author

@gbrail Some more questions

  1. Earlier I've asked about clarifications of the purpose of ZIP archive

    About zip archive, what is the purpose of distributing it ?
    Should it be published to Maven repo as artifact ? What should be the content of that file ?
    I just see no good reasons now to copy old ant behaviour since we could just zip jars produced for publishing to maven.
    Please give me some clues about this archive purpos

  2. Do you mind including support for Spock testing framework to the project together with Gradle adoption ? Apparently with another PR, not current one.

@gbrail
Copy link
Collaborator

gbrail commented May 6, 2015

OK -- I fixed that and merged. Thanks for all the work on this!

@gbrail gbrail closed this May 6, 2015
@eshepelyuk eshepelyuk deleted the gradle branch May 7, 2015 07:01
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.

Implement build system with Gradle Make Project IDE Friendly
2 participants