Skip to content

GH-55454: [R][C++] Vendor URI parser for the R library - #55461

Merged
jonkeane merged 6 commits into
apache:mainfrom
jonkeane:URI-parser-fix
Nov 10, 2026
Merged

jonkeane merged 6 commits into
apache:mainfrom
jonkeane:URI-parser-fix

Conversation

@jonkeane

@jonkeane jonkeane commented Nov 9, 2026 •

Copy link
Copy Markdown
Member

Rationale for this change

Make sure we can build on CRAN.

What changes are included in this PR?

A test for offline installations of the R package

Are these changes tested?

So far, all tests!

Are there any user-facing changes?

No

Was AI used for this PR?

In accordance to the AI generation guidelines, please disclose below whether and how AI was used in this PR.

PR code and description written by:

  • Human
  • AI

Reviewed before submission by:

  • Human
  • AI
  • Not reviewed

Copilot AI balanced review requested due to automatic review settings November 9, 2026 23:22
@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

⚠️ GitHub issue #55454 has been automatically assigned in GitHub to PR creator.

@jonkeane

jonkeane commented Nov 9, 2026

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-r-offline-minimal

@github-actions

github-actions Bot commented Nov 9, 2026

Copy link
Copy Markdown

Revision: 7b051a6

Submitted crossbow builds: ursacomputing/crossbow @ actions-19f8738864

Task Status
test-r-offline-minimal GitHub Actions

Copilot AI left a comment

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.

🟢 Approval recommended

The offline R package CI coverage change has no active review issues.

No open issues 🎉

What changed in this PR

Adds offline R package CI coverage for GH-55454, checking that CRAN-style builds fall back to a minimal configuration when dependency downloads are unavailable.

Changes:

  • Adds a minimal offline R build task with CRAN checks enabled and vignettes skipped.
  • Adds an opt-in test mode that disables DNS, verifies connectivity is unavailable, clears remote-cache settings, and restores DNS after the check.
File Description
dev/​tasks/​tasks.yml Defines the offline minimal R build task.
ci/​scripts/​r_test.sh Implements offline simulation and connectivity verification around R package checks.

Review effort: Balanced

@jonkeane
jonkeane requested a review from thisisnic as a code owner November 10, 2026 01:21
Copilot AI balanced review requested due to automatic review settings November 10, 2026 01:21
@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-r-offline-minimal

@github-actions

Copy link
Copy Markdown

Revision: cd7d74e

Submitted crossbow builds: ursacomputing/crossbow @ actions-6579dc5859

Task Status
test-r-offline-minimal GitHub Actions

Copilot AI left a comment

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.

🟡 Changes recommended

Unconditional vendoring breaks source installs from unsynced checkouts, and incorrect environment-variable overrides cause the tools test suite to fail in a clean environment.

2 open issues

Review effort: Balanced

Comment thread r/tools/nixlibs.R Resolved
Comment thread r/tools/test-nixlibs.R Resolved
Copilot AI balanced review requested due to automatic review settings November 10, 2026 01:59
@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-r-offline-minimal

@github-actions

Copy link
Copy Markdown

Revision: 3fa979f

Submitted crossbow builds: ursacomputing/crossbow @ actions-16fc506ebf

Task Status
test-r-offline-minimal GitHub Actions

Copilot AI left a comment

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.

🔵 Needs a closer look

Critical issues remain: archive cleanup can delete unrelated files in an existing DESTDIR, and source installs fail after make clean because metadata lookup assumes the removed tools/cpp tree exists.

No open issues 🎉

2 previously missed issues

In code that hasn't changed since last review

High severity Handle missing synced metadata when installing from a checkout

r/​tools/​nixlibs.R:891

After make sync-cpp && make clean, tools/vendored remains but tools/cpp is removed, so a direct source install from the checkout now aborts in get_component_names() despite find_local_source() supporting ../cpp. Skip this lookup when the synced metadata is absent, or read metadata from the selected C++ source tree. If adding a metadata-existence guard, update the mocked tests in tools/test-nixlibs.R so they still exercise vendored dependency selection without a synced tree.

High severity Restrict DESTDIR cleanup to uriparser and xsimd archives

r/​tools/​vendor_dependencies.sh:91

The optional DESTDIR can point to an existing download directory, but this loop silently deletes unrelated files, including other dependencies' archives and README files. Please restrict cleanup to older uriparser/xsimd archives instead of deleting everything outside keep.

2 resolved since last review

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:37
@jonkeane

Copy link
Copy Markdown
Member Author

@github-actions crossbow submit test-r-offline-minimal

Comment thread r/tools/nixlibs.R Resolved
@github-actions

Copy link
Copy Markdown

Revision: e758fef

Submitted crossbow builds: ursacomputing/crossbow @ actions-9b00f79b15

Task Status
test-r-offline-minimal GitHub Actions

Copilot AI left a comment

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.

🔵 Needs a closer look

Source installations can fail after make sync-cpp followed by make clean because tools/vendored remains while the metadata required by its dependency lookup is removed.

No open issues 🎉

Review effort: Balanced

Copilot AI balanced review requested due to automatic review settings November 10, 2026 13:57

Copilot AI left a comment

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.

🔵 Needs a closer look

Human assessment is needed because an unpublished critical finding indicates that source installs can fail after syncing and cleaning C++ sources.

No open issues 🎉

Review effort: Balanced

@jonkeane

jonkeane commented Nov 10, 2026 •

Copy link
Copy Markdown
Member Author

@thisisnic @amoeba this is ready for review. It vendors both uriparser and xsimd for the R package so that we can at least have a clean minimal build.

Now that we have this R-specific vendoring step, I'm separately researching how much we could get away with vendoring into the package to support more features. I suspect it's a no-go from a size perspective, but if we could get parquet support without adding a huge amount of size to the source tarball, that might be worth it.

The other option that WRE suggests is that we host these dependencies somewhere we control:

If the bundle is too large to be included in the package sources, it could be hosted at a long-term secure site under the maintainer’s control.

@amoeba amoeba left a comment

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.

+1. This looks great, and I tested it locally just for fun.

@jonkeane

Copy link
Copy Markdown
Member Author

just for fun.

Was it fun?

@jonkeane
jonkeane merged commit 4985261 into apache:main Nov 10, 2026
43 of 44 checks passed
@amoeba

amoeba commented Nov 10, 2026

Copy link
Copy Markdown
Member

just for fun.

Was it fun?

Absolutely.

@kou

kou commented Nov 11, 2026

Copy link
Copy Markdown
Member

Can we add license information for vendored dependencies to arrow/tools/LICENSE.txt in arrow_X.Y.Z.tar.gz that is submitted to CRAN?

See also: https://www.apache.org/legal/release-policy.html#license-file

When a package bundles code under several licenses, the LICENSE file MUST contain details of all these licenses. For each component which is not Apache licensed, details of the component MUST be appended to the LICENSE file. The component license itself MUST either be appended or else stored elsewhere in the package with a pointer to it from the LICENSE file, e.g. if the license is long.

See also: https://infra.apache.org/licensing-howto.html#permissive-deps

In LICENSE, add a pointer to the dependency's license within the distribution and a short note summarizing its licensing:

This product bundles SuperWidget 1.2.3, which is available under a "3-clause BSD" license. For details, see deps/superwidget/.

I know that arrow_X.Y.Z.tar.gz for CRAN is NOT an "official" release of Apache Arrow but could you follow the ASF policy?

BTW, we may need to care about the CRAN Repository Policy too:

https://cran.r-project.org/web/packages/policies.html#Source-packages

The ownership of copyright and intellectual property rights of all components of the package must be clear and unambiguous (including from the authors specification in the DESCRIPTION file). Where code is copied (or derived) from the work of others (including from R itself), care must be taken that any copyright/license statements are preserved and authorship is not misrepresented.

Preferably, an ‘Authors@R’ field would be used with ‘ctb’ roles for the authors of such code. Alternatively, the ‘Author’ field should list these authors as contributors.

Where copyrights are held by an entity other than the package authors, this should preferably be indicated via ‘cph’ roles in the ‘Authors@R’ field, or using a ‘Copyright’ field (if necessary referring to an inst/COPYRIGHTS file).

Trademarks must be respected.

(’All components’ includes any downloaded at installation or during use.)

@jonkeane

jonkeane commented Nov 11, 2026 •

Copy link
Copy Markdown
Member Author

Can we add license information for vendored dependencies to arrow/tools/LICENSE.txt in arrow_X.Y.Z.tar.gz that is submitted to CRAN?

Yes, sorry I should have caught that! I will add these separately if we need to, but I'm actually exploring vendoring more for offline builds that are more useful, and I've added some of this (in a copyrights file, though could also add it to license / move it to license too) cf https://github.com/apache/arrow/pull/55491/changes#diff-44657a281f5c907e42d0666c68ca40451306d5bad134a7af8bb5d996be0c0b6b

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.

5 participants