Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Use meson instead of configure for conda install #36489
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
Use meson instead of configure for conda install #36489
Changes from all commits
bfac057
9694046
376742a
e270392
a21655b
84ff7ff
885e2b4
f755a8e
4406868
c0cdb5f
b408311
4c8b56b
f6f27d9
73e8c86
1b61d63
3ff66b8
6c78eaa
d9e0d49
7a44082
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What does this change do? Does it have a relation to the goal of this PR?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Vscode is adding this every time I'm opening the project. I was bound to commit it at some point ;-). I doesn't hurt...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd be more comfortable with this file being local to sage-conf_meson, so as not to create expectations that Sage-the-distribution will switch to meson
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The idea in the middle to long term is to support running
meson compile
from the root directory to compile sagelib with meson (using conda). For this the meson build file needs to be there (and its pretty much convention to have such a build file in the root). Sage-the-distro supporting meson would mean that there is a meson file in the build directory, or not?There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not aware of such a plan, and I'd suggest that if you have a plan to use a new Issue in which you explain it.
From what you wrote above, it seems unlikely that it would be the same meson.build file that would be suitable for both purposes, so nothing is gained by putting it there.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's part of #34630. In the long-term you want to use of course meson-python (configured via pyproject.toml). But in the short to midterm its easier to just call meson directly. That's at least how scipy/numpy proceeded: https://github.com/scipy/scipy/pull/14847/files#diff-e42998d51257e6f84aa51ffa5f4ed1544cb12f97a94978c44f3bc281b5324d79R390-R500
And yes, the meson file added in this PR can be used for this purpose. In fact, I believe we can get ride of the conf and setup packages, but of course there still needs to be some way to provide the runtime-config (probably by just putting the generated conf file in the correct place in the builddir).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Well, #34630 has nothing to with stuff that belongs in SAGE_ROOT.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There is nothing conda-specific about the meson files, it's only that conda provides a nice fixed environment and thus is a good starting point. You can try to run
meson setup
outside of a conda environment and it should work (if you have all the necessary packages installed in the system).There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll explain and summarize once more:
configure.ac
andMakefile
do. They configure / build the Sage distribution.meson.build
next toconfigure.ac
andMakefile
, they are bound to think that using meson is an alternative for building the same thing. So this has the potential to mislead.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So if you see a hammer and a saw next to each other you are bound to see them as alternatives for building the same thing?
Conceptually,
meson setup
does the same thing asconfigure
: go through the declared dependencies and write out a config file. Similarly,meson compile
will do the same thing asmake sage-all
(or whatever the correct target is for building sagelib).It's also not technically possible to move the meson file in the root to
src
since meson'ssubdir
only walks down the tree.Anyway, I'm tired of this stupid discussion and will stop participating in it. If you think that the harm created by the meson file in the root outweighs the positives of this PR, then please go ahead and continue blocking it. Please leave the "ready for review" tag on, as there are no work items that referee and PR creator agree on (it just that you don't like a small design choice in the PR).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The target
make sage-all
does not exist.make all
, the default target, builds the Sage distribution.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually
SAGE_ROOT/tox.ini
defines the portability tests of the Sage distribution.