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

MINOR: [Docs] Update docs to point to separate Java codebase #45134

Merged
merged 4 commits into from
Jan 30, 2025

Conversation

ParthChonkar
Copy link
Contributor

@ParthChonkar ParthChonkar commented Dec 31, 2024

Rationale for this change

Post #44945 the Java implementation lives in it's own repo. Update docs
to point there.

What changes are included in this PR?

Updates to a few locations that reference old Java impl location.

Are these changes tested?

Rendered the Sphinx ones locally to check.

Are there any user-facing changes?

No

@ParthChonkar
Copy link
Contributor Author

ParthChonkar commented Dec 31, 2024

These changes are pretty trivial + are mainly for docs that aren't specifically related to java.

Though one remaining issue is that the following pages/sub-pages

still live in this repo which I assume isn't desired 🤔 .

@kou kou changed the title MINOR: [Docs] Update docs to point to separate java codebase MINOR: [Docs] Update docs to point to separate Java codebase Dec 31, 2024
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting review Awaiting review labels Dec 31, 2024
@kou
Copy link
Member

kou commented Dec 31, 2024

@github-actions crossbow submit preview-docs

Copy link

Revision: 1a0d899

Submitted crossbow builds: ursacomputing/crossbow @ actions-c3413aeb10

Task Status
preview-docs GitHub Actions

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Dec 31, 2024
@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 1, 2025
@kou
Copy link
Member

kou commented Jan 1, 2025

Could you also update cd arrow/java too in http://crossbow.voltrondata.com/pr_docs/45134/developers/java/building.html ?

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 2, 2025
@ParthChonkar
Copy link
Contributor Author

@github-actions crossbow submit preview-docs

Copy link

github-actions bot commented Jan 2, 2025

Only contributors can submit requests to this bot. Please ask someone from the community for help with getting the first commit in.
The Archery job run can be found at: https://github.com/apache/arrow/actions/runs/12575895325

@@ -144,7 +144,7 @@ Maven

.. code-block::

$ cd arrow/java
$ cd arrow-java
$ mvn generate-resources -Pgenerate-libs-jni-windows -N
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Building JNI libs with maven is broken until the pom.xml file is updated in arrow-java to avoid passing -S java to cmake among other problems.

Copy link
Member

Choose a reason for hiding this comment

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

Should we wait for apache/arrow-java#449 ?

Copy link
Contributor Author

@ParthChonkar ParthChonkar Jan 2, 2025

Choose a reason for hiding this comment

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

Yup, I'll wait on apache/arrow-java#449.

Will this order work?

Copy link
Member

Choose a reason for hiding this comment

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

Yes. Let's use the order.

@@ -293,7 +300,7 @@ Archery

.. code-block:: text

$ cd arrow
$ cd arrow-java
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Bit unfamiliar with archery, but I couldn't find any directives/targets for java-jni-manylinux-2014 in any of the dockerfiles between both repos.

Copy link
Member

Choose a reason for hiding this comment

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

We can use docker compose instead of Archery.
apache/arrow-java uses docker compose directly instead of via Archery.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah I see, after apache/arrow-java#449 I can replace the archery sections with docker compose

@ParthChonkar
Copy link
Contributor Author

Could you also update cd arrow/java too in http://crossbow.voltrondata.com/pr_docs/45134/developers/java/building.html ?

Updated + tweaked the build instructions for the JNI libraries. I commented on this review a few more problems in the "building Java" docs. Some need source fixes to be done in arrow-java - could these updates be made once the java documentation has moved to apache/arrow-java maybe?

@kou
Copy link
Member

kou commented Jan 2, 2025

@github-actions crossbow submit preview-docs

@github-actions github-actions bot added awaiting changes Awaiting changes and removed awaiting change review Awaiting change review labels Jan 2, 2025
Copy link

github-actions bot commented Jan 2, 2025

Revision: 197369e

Submitted crossbow builds: ursacomputing/crossbow @ actions-88de21002f

Task Status
preview-docs GitHub Actions

@ParthChonkar
Copy link
Contributor Author

ParthChonkar commented Jan 28, 2025

Apologies was a bit delayed here. I see that building.rst has been moved to arrow-java in https://github.com/apache/arrow-java/pull/553/files.

A significant portion of that file needs to be updated - even with the existing changes I have in this PR.

Given the above, could I leave building.rst out of this PR? The other two updates are for files that will remain in this repo.
I believe it'll make more sense to update the java/building.rst documentation iteratively in arrow-java.

@kou
Copy link
Member

kou commented Jan 29, 2025

Yes, please.

@github-actions github-actions bot added awaiting change review Awaiting change review and removed awaiting changes Awaiting changes labels Jan 30, 2025
@ParthChonkar
Copy link
Contributor Author

Reverted building.rst changes

@kou
Copy link
Member

kou commented Jan 30, 2025

@github-actions crossbow submit preview-docs

Copy link

Revision: 26a6cf8

Submitted crossbow builds: ursacomputing/crossbow @ actions-ba09884753

Task Status
preview-docs GitHub Actions

Copy link
Member

@kou kou left a comment

Choose a reason for hiding this comment

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

@kou kou merged commit aaa88e9 into apache:main Jan 30, 2025
7 checks passed
@github-actions github-actions bot added awaiting merge Awaiting merge and removed awaiting change review Awaiting change review labels Jan 30, 2025
Copy link

After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit aaa88e9.

There were 8 benchmark results with an error:

There were no benchmark performance regressions. 🎉

The full Conbench report has more details. It also includes information about 5 possible false positives for unstable benchmarks that are known to sometimes produce them.

lriggs pushed a commit to lriggs/arrow that referenced this pull request Jan 30, 2025
…45134)

### Rationale for this change
Post  apache#44945 the Java implementation lives in it's own repo. Update docs
to point there. 

### What changes are included in this PR?
Updates to a few locations that reference old Java impl location.

### Are these changes tested?
Rendered the Sphinx ones locally to check.  

### Are there any user-facing changes?
No

Lead-authored-by: parthchonkar <[email protected]>
Co-authored-by: Parth Chonkar <[email protected]>
Co-authored-by: Sutou Kouhei <[email protected]>
Signed-off-by: Sutou Kouhei <[email protected]>
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.

2 participants