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

Build enhancement for Conda #722

Merged
merged 5 commits into from
Jun 21, 2024
Merged

Conversation

eddelbuettel
Copy link
Contributor

PR #710 (with SC 47400) brought in a small extension which included small Makevars (the Makefile snippet used by the R package) changes. As was seen in the subsequent release this was not fully Conda-compliant but additional tests helped determine a very small additional fix. A review by @jdblischak (who already tested this) would be nice but I cannot currently request one.

@jdblischak
Copy link

I tested this PR on my feedstock fork (commit, build)

  • Good news: Enables the VFS-based read and write connections (PR Support VFS-based read and write connections #710) on osx-64
  • Bad news: As I first discovered Friday when testing an earlier iteration, this recent change causes a test failure only on linux-aarch64 R 4.3. This is bizarre since it passes on R 4.2. Also, I triggered a feedstock build this morning off of the current main branch, and the linux-aarch64 R 4.3 tests passed fine, so it isn't due to an update in the Azure image

I still think we should merge this PR since it unblocks this new feature for the osx-64 conda binary. However, we'll need to do some troubleshooting before the next r-tiledb-feedstock release in order to figure out which test(s) to skip on linux-aarch64. From comparing the R 4.2 and 4.3 logs, it appears the error occurs on the 20th test in test_fragmentinfo.R

@jdblischak
Copy link

I suspect the error is happening here. Any reason to suspect this code would be consistently problematic for R 4.3 (but not 4.2) and linux-aarch64 (but not linux-64)? Worst case I'll just add a patch to the conda recipe to skip on linux-aarch64

D2 <- data.frame(keys = 11:20,
groups = replicate(10, paste0(sample(LETTERS[1:4], 3), collapse="")),
vals = sample(100, 10, TRUE))
arr <- tiledb_array(uri, "WRITE")
arr[] <- D2
array_consolidate(uri) # written twice so consolidate
rm(fraginf)
fraginf <- tiledb_fragment_info(uri)

@eddelbuettel
Copy link
Contributor Author

Two conversations here:

  1. The PR itself which adds build to Conda breaks macOS builds (once that ever-so-helpful decides to unqueue itself and actually run anything). Ball in my court for this, will try to work out with mac-builder or rhub what needs to change.
  2. The aarch64 aka arm64 issue: Strictly no idea, and 'old enough code' and tests to have been subjected to R 4.3.* before. No idea what's going on, no ability to do any work as I do not have access to aarch64 aka arm64 right now.

@johnkerl

This comment was marked as off-topic.

@eddelbuettel

This comment was marked as off-topic.

@johnkerl

This comment was marked as off-topic.

@jdblischak

This comment was marked as off-topic.

@johnkerl

This comment was marked as off-topic.

@eddelbuettel

This comment was marked as off-topic.

@johnkerl

This comment was marked as off-topic.

@johnkerl

This comment was marked as off-topic.

@eddelbuettel
Copy link
Contributor Author

eddelbuettel commented Jun 18, 2024

As owner of this issue and PR I took the liberty of marking the very interesting and passionate (but not really relevant here) discussion of another build platform as off-topic.

The current state (and primary puzzle) is that enabling Conda killed macOS. Bad trade-off.

@eddelbuettel
Copy link
Contributor Author

So we now have the CI Actions back to ✔️ but there may be remaining Conda questions.

@eddelbuettel eddelbuettel force-pushed the de/sc-49640/conda_build_refinement branch from a33e587 to d9a0019 Compare June 20, 2024 21:44
@jdblischak
Copy link

Testing the latest change that uses the env var CONDA_BUILD to configure the setup in the Azure macOS conda builds

https://dev.azure.com/jdblischak/feedstock-builds/_build/results?buildId=863&view=results

@eddelbuettel
Copy link
Contributor Author

Yay! Looks like we won that battle:

image

The remaining fail is not new, and a continued puzzle. So onwards, gonna merge this now.

@eddelbuettel eddelbuettel merged commit 81ac0f8 into master Jun 21, 2024
8 checks passed
@eddelbuettel eddelbuettel mentioned this pull request Jun 21, 2024
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