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

[SPARK-20264][SQL] asm should be non-test dependency in sql/core #17574

Closed
wants to merge 1 commit into from

Conversation

rxin
Copy link
Contributor

@rxin rxin commented Apr 8, 2017

What changes were proposed in this pull request?

sq/core module currently declares asm as a test scope dependency. Transitively it should actually be a normal dependency since the actual core module defines it. This occasionally confuses IntelliJ.

How was this patch tested?

N/A - This is a build change.

@SparkQA
Copy link

SparkQA commented Apr 8, 2017

Test build #75618 has started for PR 17574 at commit 2a03188.

@SparkQA
Copy link

SparkQA commented Apr 8, 2017

Test build #3648 has finished for PR 17574 at commit 2a03188.

  • This patch fails Spark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented Apr 8, 2017

retest this please.

@viirya
Copy link
Member

viirya commented Apr 8, 2017

Do we need it as a normal dependency? Looks like sql/core doesn't use it and the building works without this dependency. Sorry if I am missing something.

@SparkQA
Copy link

SparkQA commented Apr 8, 2017

Test build #75623 has finished for PR 17574 at commit 2a03188.

  • This patch fails PySpark unit tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

@viirya
Copy link
Member

viirya commented Apr 8, 2017

retest this please.

@SparkQA
Copy link

SparkQA commented Apr 8, 2017

Test build #75626 has finished for PR 17574 at commit 2a03188.

  • This patch passes all tests.
  • This patch merges cleanly.
  • This patch adds no public classes.

Copy link
Member

@gatorsmile gatorsmile left a comment

Choose a reason for hiding this comment

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

LGTM

@gatorsmile
Copy link
Member

Thanks! Merging to master/2.1

asfgit pushed a commit that referenced this pull request Apr 10, 2017
## What changes were proposed in this pull request?
sq/core module currently declares asm as a test scope dependency. Transitively it should actually be a normal dependency since the actual core module defines it. This occasionally confuses IntelliJ.

## How was this patch tested?
N/A - This is a build change.

Author: Reynold Xin <rxin@databricks.com>

Closes #17574 from rxin/SPARK-20264.

(cherry picked from commit 7bfa05e)
Signed-off-by: Xiao Li <gatorsmile@gmail.com>
@asfgit asfgit closed this in 7bfa05e Apr 10, 2017
@gatorsmile
Copy link
Member

@viirya Based on the code change history, #13642 removed the usage of ASM in the test case SQLMetricsSuite.scala. Thus, it is safe to remove the test dependency.

I am not 100% sure whether sql/core has any dependence on it. What I saw in the history is the PR #9512 added it for testing only.

@viirya
Copy link
Member

viirya commented Apr 10, 2017

@gatorsmile Thanks for the search. I don't see any usage of it in sql/core now. It is only used in core, repl, graphx. So I am wondering if we can completely remove it from the dependency.

@gatorsmile
Copy link
Member

You can make a try.

@rxin
Copy link
Contributor Author

rxin commented Apr 10, 2017

Meh let's not bother. There isn't any harm in the current setup since it's already a transitive dependency. Why waste time on those?

@srowen
Copy link
Member

srowen commented Apr 10, 2017

BTW making it compile scope means it is still also in test scope, which is fine.
I actually don't see xbean as a transitive dependency otherwise? at least from Maven and SBT as best I know how to use them to find out.

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