Skip to content

Gradle Builds: Change provided to testCompile#1394

Merged
benjchristensen merged 4 commits into
ReactiveX:masterfrom
gliptak:scalabindings
Jul 10, 2014
Merged

Gradle Builds: Change provided to testCompile#1394
benjchristensen merged 4 commits into
ReactiveX:masterfrom
gliptak:scalabindings

Conversation

@gliptak
Copy link
Copy Markdown
Contributor

@gliptak gliptak commented Jun 27, 2014

Gradle now generates correct eclipse dependencies

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1336 FAILURE
Looks like there's a problem with this pull request

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

@benjchristensen do you know why use provide here? Looks most of other projects also use provided. I suppose testCompile may be better for these test libraries. I changed provided to testCompile in rxjava-scala. Looks it works.

dependencies {
    compile 'org.scala-lang:scala-library:2.10.+'

    compile project(':rxjava-core')

    testCompile 'junit:junit-dep:4.10'
    testCompile 'org.mockito:mockito-core:1.8.5'
    testCompile 'org.scalatest:scalatest_2.10:1.9.1'
}

tasks.compileScala {  
    classpath = classpath + (configurations.compile + configurations.provided)
}

tasks.compileExamplesScala {  
    classpath = classpath + files(compileScala.destinationDir) + (configurations.compile + configurations.testCompile)
}

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

The use of provided dates back to over a year ago and is no longer the correct approach. We should move to testCompile. I haven't merged this yet because it failed to build and wasn't obvious why. Also, if we're changing it, we should completely remove provided.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This is all voodoo for me, but I do know that whenever I refresh my repo, I need to install everything that is "provided" by hand, which is not what I would like and expect.

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1361 SUCCESS
This pull request looks good

@gliptak
Copy link
Copy Markdown
Contributor Author

gliptak commented Jul 4, 2014

Cleaned up more provided references

@samuelgruetter
Copy link
Copy Markdown
Contributor

LGTM.
With current master, running ./gradlew eclipse generates a project file which does not contain the scalatest JAR, so Eclipse cannot build the project. I've been annoyed by this for a long time already, and just fixed it by manually adding the scalatest JAR, because I did not know how to fix the gradle configuration. This PR fixes the problem, so that ./gradlew eclipse generates a correct project file.
Thank you @gliptak :-)

@zsxwing
Copy link
Copy Markdown
Member

zsxwing commented Jul 6, 2014

Could you also fix other build.gradles? Thanks.

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1363 SUCCESS
This pull request looks good

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

For Android, here should be provided because rxjava-android needs to work with different Android versions.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

@zsxwing This specifies how this project is built (and that during the build these dependencies can be downloaded from Maven repo). You can still use different android jars for runtime linkage.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Actually, it's an Android special one. Using these libraries in android development is much more like using JDK runtime. It's better that we don't add these dependencies to rxjava-android. gradle android plugin will set up the environment according to the settings for an android project, such as:

apply plugin: 'android'

android {
    buildToolsVersion "19.0.3"
    compileSdkVersion 19
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

So you suggest removing both com.google.android references?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

No. I suggest changing back to provided.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Waiting on conclusion to this one.

/cc @mttkay

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1370 FAILURE
Looks like there's a problem with this pull request

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1371 SUCCESS
This pull request looks good

@gliptak
Copy link
Copy Markdown
Contributor Author

gliptak commented Jul 8, 2014

@zsxwing with this last commit I removed support for provided in the general build process, so I would like to suggest staying with compile for now. I looked into switching to http://tools.android.com/tech-docs/new-build-system/user-guide The changes will be fairly invasive, as the plugin android conflicts with plugin java, so I would like to work that as a separate pull request.

@zsxwing
Copy link
Copy Markdown
Member

zsxwing commented Jul 8, 2014

@gliptak sorry that I may explain unclearly. I mean let's keep the following lines:

provided 'com.google.android:android:4.0.1.2'
provided 'com.google.android:support-v4:r7'

For the android gradle plugin, I mean when some projects depends on rxjava-android, they can use android gradle plugin. I don't mean that rxjava-android needs to use it. So for rxjava-android, I suggest we only need to do the following changes:

 -    provided 'junit:junit-dep:4.10'
 -    provided 'org.mockito:mockito-core:1.8.5'
 -    provided 'org.robolectric:robolectric:2.2'
 +    testCompile 'junit:junit-dep:4.10'
 +    testCompile 'org.mockito:mockito-core:1.8.5'
 +    testCompile 'org.robolectric:robolectric:2.2'

@benjchristensen benjchristensen changed the title Set scalatest as testCompile dependency Gradle Builds: Change provided to testCompile Jul 8, 2014
@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1376 FAILURE
Looks like there's a problem with this pull request

@gliptak
Copy link
Copy Markdown
Contributor Author

gliptak commented Jul 8, 2014

Hmm ... The build failed with a possibly unrelated test failure (I started seeing failures locally after rebasing on the backpressure changes)

@benjchristensen
Copy link
Copy Markdown
Member

Ignore rx.internal.operators.OperatorPivotTest.testPivotEvenAndOdd. It has some non-determinism that I haven't been able to figure out.

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Could you indent these new test files with 4-spaces rather than tabs?

@cloudbees-pull-request-builder
Copy link
Copy Markdown

RxJava-pull-requests #1378 SUCCESS
This pull request looks good

@zsxwing
Copy link
Copy Markdown
Member

zsxwing commented Jul 10, 2014

LGTM. Tested most projects in eclipse and IntelliJ IDEA. Didn't test rxjava-clojure, rxjava-groovy, rxjava-jruby and rxjava-kotlin because I didn't install these language plugins in my IDES.

Thanks, @gliptak

@mttkay
Copy link
Copy Markdown
Contributor

mttkay commented Jul 10, 2014

👍

benjchristensen added a commit that referenced this pull request Jul 10, 2014
Gradle Builds: Change provided to testCompile
@benjchristensen benjchristensen merged commit 14f3f51 into ReactiveX:master Jul 10, 2014
@gliptak gliptak deleted the scalabindings branch July 16, 2014 13:16
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.

7 participants