Skip to content

Meson: automatically install into venv if activated #39954

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

Merged
merged 2 commits into from
Apr 29, 2025

Conversation

tobiasdiez
Copy link
Contributor

Makes the meson experience a bit smoother by no longer requiring that a manual prefix is specified (it still can be specified if wanted). So meson install will just install into the current venv, the same as pip install ..

📝 Checklist

  • The title is concise and informative.
  • The description explains in detail what this PR is about.
  • I have linked a relevant issue or discussion.
  • I have created tests covering the changes.
  • I have updated the documentation and checked the documentation preview.

⌛ Dependencies

Copy link

github-actions bot commented Apr 17, 2025

Documentation preview for this PR (built with commit 3f166c3; changes) is ready! 🎉
This preview will update shortly after each push to this PR.

@user202729
Copy link
Contributor

user202729 commented Apr 17, 2025

Personally I just use editable install and pip, but the change looks safe enough. (according to the documentation the default is prefix only for backwards compatibility purpose) Haven't tested though.

To check: Is the documentation change still correct i.e. are the listed meson commands what pip uses under the hood after the change?

Co-authored-by: user202729 <25191436+user202729@users.noreply.github.com>
@dimpase
Copy link
Member

dimpase commented Apr 18, 2025

By the way, what's this

test-new Expected — Waiting for status to be reported

pending status for this CI job I see across a number of PR?

Copy link
Member

@dimpase dimpase left a comment

Choose a reason for hiding this comment

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

LGTM. I'll wait under docbuild finishes, to flip this to positive (something I can finally do myself)

@user202729
Copy link
Contributor

By the way, what's this

test-new Expected — Waiting for status to be reported

pending status for this CI job I see across a number of PR?

After #39641 , test-new no longer exists. Not sure why GitHub still shows the check in the UI.

@tobiasdiez
Copy link
Contributor Author

tobiasdiez commented Apr 19, 2025

By the way, what's this

test-new Expected — Waiting for status to be reported

pending status for this CI job I see across a number of PR?

After #39641 , test-new no longer exists. Not sure why GitHub still shows the check in the UI.

It's a "required check". Someone of the @sagemath/core admins need to change the branch protection rules to no longer include the "test-new" workflow, but instead all the meson workflows. Thanks in advance!


Thanks for the review! Flipping to positive review since the docbuild finished without error.


.. CODE-BLOCK:: shell-session

$ PYTHONPATH=/desired/install/path ./sage
Copy link
Contributor

@user202729 user202729 Apr 19, 2025

Choose a reason for hiding this comment

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

are you sure this actually works, in the previous version the PYTHONPATH is set to …/site-packages (I haven't tested either)

(there's also the alternative of removing the note entirely… does anyone need it?)

vbraun pushed a commit to vbraun/sage that referenced this pull request Apr 21, 2025
sagemathgh-39954: Meson: automatically install into venv if activated
    
<!-- ^ Please provide a concise and informative title. -->
<!-- ^ Don't put issue numbers in the title, do this in the PR
description below. -->
<!-- ^ For example, instead of "Fixes sagemath#12345" use "Introduce new method
to calculate 1 + 2". -->
<!-- v Describe your changes below in detail. -->
<!-- v Why is this change required? What problem does it solve? -->
<!-- v If this PR resolves an open issue, please link to it here. For
example, "Fixes sagemath#12345". -->

Makes the meson experience a bit smoother by no longer requiring that a
manual prefix is specified (it still can be specified if wanted). So
`meson install` will just install into the current venv, the same as
`pip install .`.

### 📝 Checklist

<!-- Put an `x` in all the boxes that apply. -->

- [ ] The title is concise and informative.
- [ ] The description explains in detail what this PR is about.
- [ ] I have linked a relevant issue or discussion.
- [ ] I have created tests covering the changes.
- [ ] I have updated the documentation and checked the documentation
preview.

### ⌛ Dependencies

<!-- List all open PRs that this PR logically depends on. For example,
-->
<!-- - sagemath#12345: short description why this is a dependency -->
<!-- - sagemath#34567: ... -->
    
URL: sagemath#39954
Reported by: Tobias Diez
Reviewer(s): Dima Pasechnik, user202729
@vbraun vbraun merged commit c1a1bc7 into sagemath:develop Apr 29, 2025
26 of 27 checks passed
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.

4 participants