Skip to content

Use scikit-build #673

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

Open
wants to merge 9 commits into
base: main
Choose a base branch
from
Open

Use scikit-build #673

wants to merge 9 commits into from

Conversation

pablogsal
Copy link
Member

  • Try to respect RPATHS of calling dlopen modules with dlinfo
  • Use scikit-build-core

Issue number of the reported bug or feature request: #

Describe your changes
A clear and concise description of the changes you have made.

Testing performed
Describe the testing you have performed to ensure that the bug has been addressed, or that the new feature works as planned.

Additional context
Add any other context about your contribution here.

@pablogsal pablogsal force-pushed the build branch 2 times, most recently from a73f4f0 to 638c30d Compare September 3, 2024 12:15
@pablogsal pablogsal changed the title build Use scikit-build Sep 3, 2024
@codecov-commenter
Copy link

codecov-commenter commented Sep 18, 2024

Codecov Report

Attention: Patch coverage is 93.75000% with 1 line in your changes missing coverage. Please review.

Project coverage is 93.54%. Comparing base (457365a) to head (44ec99f).
Report is 20 commits behind head on main.

Files with missing lines Patch % Lines
tests/integration/test_tracking.py 80.00% 1 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #673      +/-   ##
==========================================
+ Coverage   92.91%   93.54%   +0.63%     
==========================================
  Files          95       97       +2     
  Lines       11475    11558      +83     
  Branches     2114     2114              
==========================================
+ Hits        10662    10812     +150     
+ Misses        813      746      -67     
Flag Coverage Δ
cpp 93.54% <93.75%> (+0.63%) ⬆️
python_and_cython 93.54% <93.75%> (+0.63%) ⬆️

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

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@godlygeek godlygeek force-pushed the build branch 3 times, most recently from 4dd729a to 73cd67e Compare September 25, 2024 00:28
pablogsal and others added 9 commits September 25, 2024 13:36
Signed-off-by: Pablo Galindo <pablogsal@gmail.com>
These environment variables previously allowed us to conditionally
enable extra safety checks in our Cython code, or conditionally disable
optimizations that might break tools like Valgrind. Now, we control this
based on the CMake build type, so that release builds have the extra
optimizations and debug builds don't, and so that debug builds have the
extra safety checks and release builds don't.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
Some tests were erroneously removing all environment variables from the
environment and passing along an environment consisting of only a single
variable. Since this removes environment variables like $PATH, this
breaks automatic rebuilds -- they can't find the compiler!

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
This test failed in CI because the same time was captured in both
`end_time` and `start_time`. There's no reason to assume that this took
more than a millisecond, or that the system clock can give us better
than millisecond granularity, so it seems more reasonable to simply
check that the end time is no earlier than the start time. Technically
even that assumption could be violated, since we're not using
a monotonic clock, but oh well, this is just tests.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
We meant to ignore any Makefile generated by CMake, but we shouldn't be
ignoring our own hand-written Makefile.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
`setuptools` uses CFLAGS for both C and C++ builds, but CMake
distinguishes between them, and only uses CXXFLAGS for C++ builds.
Whenever we want to set flags that affect both C and C++ builds, ensure
that we set both CFLAGS and CXXFLAGS.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
With `scikit-build-core`, we don't have any way to produce an entry
point script called `memray3.N` (where `3.N` is the Python version
Memray was installed for). Drop references to it from the docs.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
With an editable `scikit-build-core` install, shared libraries are not
in the same directory as Python files, so we can no longer locate it
relative to `__init__.py`.

Signed-off-by: Matt Wozniski <mwozniski@bloomberg.net>
@godlygeek
Copy link
Contributor

This is missing the npm install and npm run-script build steps, I think

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.

3 participants