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

[python] Fix and test setup.py flags to external shared objects #2221

Merged
merged 1 commit into from
Mar 6, 2024

Conversation

jdblischak
Copy link
Collaborator

@jdblischak jdblischak commented Mar 6, 2024

Issue and/or context:

Changes:

  • Restores the flags --tiledb and --libtiledbsoma that are passed to setup.py

Notes for Reviewer:

Here's what happened. PR #1937 fixed the shared object copying when an external libtiledb or libtiledbsoma is provided via the env vars TILEDB_PATH or TILEDBSOMA_PATH. Unfortunately, it broke the command-line flags --tiledb and --libtiledbsoma. It overwrites them with None. This is why the tiledbsoma-feedstock nightlies immediately started failing after it was merged, which is the only place where we still build tiledbsoma-py with setup.py.

We assumed there was a macOS-specific problem because 1) the linux-64 feedstock build continued to pass, and 2) #1937 only tested on linux and not macOS. However, after I added a macOS job in #2220, we knew it was not specific to macOS since macOS also properly copies the objects when using the env vars.

I figured it out by investigating the differences between linux and macOS in setup.py. It turns out that linux has a fall-back method to search for a system installation of libtiledbsoma. This is the only reason that the linux-64 feedstock build passed.

if os.name == "posix" and sys.platform != "darwin":
path = find_libtiledbsoma_full_path_on_linux(lib_name)

In order to catch this type of regression in this repository, I added an extra job to my python-so-copying workflow that confirms the flags --tiledb and --libtiledbsoma are functional. To emulate the feedstock build, I delete cmake from the runner. Thus if tiledbsoma-py is unable to use the command-line flags and tries to fall back to building libtiledbsoma from source with cmake, it will fail (which I confirmed on a branch in my fork by temporarily reverting my fixes to setup.py).

See also #2219 for tracking

@johnkerl johnkerl changed the title [python] Fix and test setup.py flags to external shared objects [python] Fix and test setup.py flags to external shared objects Mar 6, 2024
@johnkerl
Copy link
Member

johnkerl commented Mar 6, 2024

See also #2210 for troubleshooting context.

Copy link
Member

@johnkerl johnkerl left a comment

Choose a reason for hiding this comment

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

🚢

Copy link

codecov bot commented Mar 6, 2024

Codecov Report

Merging #2221 (0a9b015) into main (793818d) will decrease coverage by 6.47%.
The diff coverage is n/a.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #2221      +/-   ##
==========================================
- Coverage   78.49%   72.02%   -6.47%     
==========================================
  Files         137      103      -34     
  Lines       10683     6875    -3808     
  Branches      215      215              
==========================================
- Hits         8386     4952    -3434     
+ Misses       2198     1824     -374     
  Partials       99       99              
Flag Coverage Δ
libtiledbsoma 67.44% <ø> (ø)
python ?
r 74.69% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

Components Coverage Δ
python_api ∅ <ø> (∅)
libtiledbsoma 48.85% <ø> (ø)

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.

[ci] dylib-copy logic appears to be broken on MacOS
2 participants