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

[SEDONA-473] Remove ucar from spark-shaded pom #1240

Merged
merged 5 commits into from
Feb 22, 2024

Conversation

iGN5117
Copy link
Contributor

@iGN5117 iGN5117 commented Feb 18, 2024

Did you read the Contributor Guide?

Is this PR related to a JIRA ticket?

What changes were proposed in this PR?

  • Remove ucar from spark-shaded pom

How was this patch tested?

  • Passed existing tests

Did this PR include necessary documentation updates?

  • No, this PR does not affect any public API so no need to change the docs.

Copy link
Member

@Kontinuation Kontinuation left a comment

Choose a reason for hiding this comment

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

This does not resolve SEDONA-473, since cdm-core is introduced by the parent POM, not the shaded POM. Actually the POM published to maven central for spark-shaded is a dependency-reduced POM, which does not have dependencies in compile scope (See https://repo1.maven.org/maven2/org/apache/sedona/sedona-spark-shaded-3.5_2.12/1.5.1/sedona-spark-shaded-3.5_2.12-1.5.1.pom).

The correct way to resolve this issue is to remove https://github.com/apache/sedona/blob/master/pom.xml#L132-L135 in the parent POM.

@iGN5117
Copy link
Contributor Author

iGN5117 commented Feb 20, 2024

This does not resolve SEDONA-473, since cdm-core is introduced by the parent POM, not the shaded POM. Actually the POM published to maven central for spark-shaded is a dependency-reduced POM, which does not have dependencies in compile scope (See https://repo1.maven.org/maven2/org/apache/sedona/sedona-spark-shaded-3.5_2.12/1.5.1/sedona-spark-shaded-3.5_2.12-1.5.1.pom).

The correct way to resolve this issue is to remove https://github.com/apache/sedona/blob/master/pom.xml#L132-L135 in the parent POM.

@Kontinuation thanks for checking this PR. I was confused if I would need to keep ucar in the parent pom to support NetCDF functionality, but even removing it makes no difference.
I've reverted my earlier commit and removed ucar from sedona/pom.xml now. Please approve if it looks good!

@Kontinuation
Copy link
Member

@Kontinuation thanks for checking this PR. I was confused if I would need to keep ucar in the parent pom to support NetCDF functionality, but even removing it makes no difference. I've reverted my earlier commit and removed ucar from sedona/pom.xml now. Please approve if it looks good!

Now it should be good, but I prefer removing the unused dependency instead of commenting it out.

@iGN5117
Copy link
Contributor Author

iGN5117 commented Feb 20, 2024

@Kontinuation thanks for checking this PR. I was confused if I would need to keep ucar in the parent pom to support NetCDF functionality, but even removing it makes no difference. I've reverted my earlier commit and removed ucar from sedona/pom.xml now. Please approve if it looks good!

Now it should be good, but I prefer removing the unused dependency instead of commenting it out.

Yes ofcourse, the comment was temporary. Removed it completely.

@jiayuasu jiayuasu added this to the sedona-1.6.0 milestone Feb 22, 2024
@jiayuasu jiayuasu merged commit a1d1a45 into apache:master Feb 22, 2024
46 checks passed
jiayuasu pushed a commit that referenced this pull request Apr 28, 2024
* remove ucar from pom.xml

* Revert "remove ucar from pom.xml"

This reverts commit 67692a6.

* remove ucar from sedona pom.xml

* remove comment for ucar
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants