Skip to content

Test pip editable install with meson #39369

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.

Sign up for GitHub

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

Merged
merged 20 commits into from
Mar 9, 2025
Merged

Test pip editable install with meson #39369

Changes from 19 commits
Commits
Show all changes
20 commits
Select commit Hold shift + click to select a range
7e769e7
Test pip editable install with meson
user202729 Jan 22, 2025
fc4d1a3
Add editable status to name
user202729 Jan 22, 2025
99e9221
Fix bug?
user202729 Jan 22, 2025
ec4531f
Fix another bug
user202729 Jan 22, 2025
fba9bf7
Hope this works
user202729 Jan 22, 2025
4d5e89d
Better name
user202729 Jan 23, 2025
0b37a16
Merge remote-tracking branch 'upstream/develop' into meson-try-editable
user202729 Jan 28, 2025
c0e4472
Avoid issues with artifact name collision
user202729 Feb 1, 2025
e366e2a
Use import_module instead of find_spec
user202729 Feb 1, 2025
1b55c8e
Merge remote-tracking branch 'upstream/develop' into meson-try-editable
user202729 Feb 11, 2025
40c0b19
Fix more doctests in meson_editable install
user202729 Feb 11, 2025
0b55818
Merge branches 'fix-more-doctests-meson-editable' and 'fix-load-submo…
user202729 Feb 11, 2025
88ecd7a
Revert "Fix more doctests in meson_editable install"
user202729 Feb 12, 2025
85fb2e6
Fix more doctests in meson_editable install
user202729 Feb 11, 2025
cace8b2
Merge branches 'sort-filter-walk-packages' and 'sage-getfile-2' into …
user202729 Feb 12, 2025
b1606b5
Workaround for test-new failure
user202729 Feb 12, 2025
3d38c95
Merge branch 'develop' into meson-try-editable
user202729 Feb 22, 2025
b14e2e9
Only run 2/4 combinations in pull_request
user202729 Mar 3, 2025
13ec342
Merge remote-tracking branch 'upstream/develop' into meson-try-editable
user202729 Mar 3, 2025
43142c7
Apply suggestions
user202729 Mar 8, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 20 additions & 4 deletions .github/workflows/ci-meson.yml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,14 +16,28 @@ concurrency:

jobs:
test:
name: Conda (${{ matrix.os }}, Python ${{ matrix.python }})
name: Conda (${{ matrix.os }}, Python ${{ matrix.python }}${{ matrix.editable && ', editable' || '' }})
runs-on: ${{ matrix.os }}-latest

strategy:
fail-fast: false
matrix:
os: [ubuntu]
python: ['3.11', '3.12']
editable: [false, true]
Copy link
Contributor

Choose a reason for hiding this comment

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

This now makes it hard to extend the matrix by other OS for PRs (where you would like to run all non-editable versions for all pythons and all systems).

Does the following work?

editable: 
- false
- ${{ github.event_name != 'pull_request' }} # PR: false (which is then ignored), Push: true

include: 
          - os: ubuntu
            python: 3.12
            editable: true # One additional editable run for PRs

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't understand what combination you are trying to do. The way it currently works is

in PR, only run the combinations listed below; otherwise, run all combinations

As long as in non-PR you want to run all combinations you can just edit the matrix. Hopefully you don't need to run too many combinations in pull request that you can afford listing each configuration explicitly.

So for example if you were to add windows and mac to the os: [ubuntu], then you could modify the list below to e.g.

        include:
          - os: ubuntu
            python: 3.11
            editable: false
          - os: mac
            python: 3.12
            editable: false
          - os: windows
            python: 3.12
            editable: true

or something like that. (which is probably sufficient right? Each factor got tested at least once)

In your suggestion, on push all combinations will be run, and on pull request 3 combinations will be run (which also work, I think).

Copy link
Contributor

Choose a reason for hiding this comment

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

Hopefully you don't need to run too many combinations in pull request that you can afford listing each configuration explicitly.

That's the problem. Once we add macos and windows to the matrix, it will run 3 (os) x 3 (python) systems for each PR (with proper caching this hopefully isn't too much total CI time). I prefer not to specify all of these combinations explicitly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think one problem is do you really want to run 10 per PR. It seems like 3 is enough (see my suggestion above).

Although… I think if it gets sufficiently complicated it might be better to use full-blown code to generate the include list. (Does whatever language that GitHub Actions use have a for loop or list comprehension or map etc.? Seems not.)

e.g. https://docs.github.com/en/actions/writing-workflows/choosing-what-your-workflow-does/running-variations-of-jobs-in-a-workflow#example-using-an-output-to-define-two-matrices could be used. But then this implies we need a separate script written in something like Python (is this preinstalled?) to generate the JSON.

Is it worth it or is this overengineering?

Copy link
Contributor

Choose a reason for hiding this comment

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

Is it worth it or is this overengineering?

It's a tad too complicated in my opinion.

I think one problem is do you really want to run 10 per PR. It seems like 3 is enough

In my opinion, issues should be caught at the PR level. And the only way to observe a bug, say on MacOS with Python 3.11, is to run exactly that combination. Preferably, it would run only once for the merge and not at every push to a PR but that's not possible with our current setup.

But anyway, for now I would prefer a solution that adds exactly one "editable" run for PRs (and perhaps for all Python versions on push to develop).

is-pull-request:
- ${{ github.event_name == 'pull_request' }}
# trick from https://github.com/orgs/community/discussions/26253#discussioncomment-6745038
# in PR, only run the combinations listed below; otherwise, run all combinations
exclude:
- is-pull-request: true
include:
- os: ubuntu
python: 3.11
editable: false
- os: ubuntu
python: 3.12
editable: true

steps:
- uses: actions/checkout@v4
Expand Down Expand Up @@ -70,7 +84,7 @@ jobs:
export CC="ccache $CC"
export CXX="ccache $CXX"
# Use --no-deps and pip check below to verify that all necessary dependencies are installed via conda
pip install --no-build-isolation --no-deps --config-settings=builddir=builddir . -v
pip install --no-build-isolation --no-deps --config-settings=builddir=builddir ${{ matrix.editable && '--editable' || '' }} . -v

- name: Check update-meson
# this step must be after build, because meson.build creates a number of __init__.py files
Expand All @@ -93,12 +107,14 @@ jobs:
shell: bash -l {0}
run: |
# We don't install sage_setup, so don't try to test it
rm -R ./src/sage_setup/
# If editable then deleting the directory will cause sage to detect rebuild, which will cause ninja to fail
# so we don't delete the directory in this case
${{ matrix.editable && 'true' || 'rm -R ./src/sage_setup/' }}
./sage -t --all -p4 --format github

- name: Upload log
uses: actions/[email protected]
if: failure()
with:
name: ${{ runner.os }}-meson-${{ matrix.python }}-log
name: ${{ runner.os }}-meson-${{ matrix.python }}${{ matrix.editable && '-editable' || '' }}-log
path: builddir/meson-logs/
Loading