Repository navigation
Conversation
DougBurke
marked this pull request as draft
January 8, 2024 22:57
DougBurke
force-pushed
the
test-mesonpy
branch
from
October 9, 2024 20:24
8e3634c to
c34a9f9
Compare
Contributor
Author
|
Rebased now that 4.17.0 has been released (to pick up a number of build changes and some of the fixes that were orignially created for this PR but then git merged in other PRs). |
This was referenced Feb 14, 2025
DougBurke
force-pushed
the
test-mesonpy
branch
from
February 26, 2025 14:17
c34a9f9 to
0fdcbfe
Compare
Contributor
Author
|
Rebased as I've made some progress on providing optional support (and removing some hard-coded assumptions), but still a long way to go. |
DougBurke
force-pushed
the
test-mesonpy
branch
14 times, most recently
from
February 28, 2025 16:05
921fb0d to
e5bc5b7
Compare
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #1948 +/- ##
==========================================
+ Coverage 87.50% 87.54% +0.03%
==========================================
Files 84 84
Lines 29957 29954 -3
Branches 3832 3832
==========================================
+ Hits 26213 26222 +9
+ Misses 3634 3622 -12
Partials 110 110
... and 1 file with indirect coverage changes Continue to review full report in Codecov by Sentry.
🚀 New features to boost your workflow:
|
DougBurke
force-pushed
the
test-mesonpy
branch
from
February 28, 2025 17:55
e5bc5b7 to
098dc80
Compare
This may help: https://mesonbuild.com/Machine-files.html You can specify project options as well as which program binaries to use for things like the C/C++ compiler, pkg-config paths, etc. (Sorry, can't take a closer look at the meson.build changes right now -- the files changed tab is so big it grinds my browser to a halt.) |
DougBurke
force-pushed
the
test-mesonpy
branch
9 times, most recently
from
March 5, 2025 03:58
43a3123 to
2d3016b
Compare
This was referenced Mar 21, 2025
This was referenced May 27, 2025
Change the location of the conda environment from within
the sherpa source directory - as given by the github.workspace
value - to its parent directory.
This turns out to be important if we change to a different
build system, such as meson-python [1]. It also seems to make
sense anyway; that is, make sure the condea environment is
separate from the Sherpa code.
I also apply this for the MacOS SDK location, for similar reasons.
Two questions are
a) is the parent directory a sensible location; I can't see why not
b) is there a better way to identify this location (or, some other
location, following point a) as at present I just hard-code this
into the yaml as "${{ github.workspace }}/../<value>"
and also in setup_conda.sh for the macOS code (the macOS SDK)
---
[1] For example
rgommers/scipy#82 (comment)
Ensure that FFTW3 is available on the CI machines (this does not set the code up to build against these libraries).
With this I can
pip install .
and then in another directory
sherpa_test -n auto
Editable installs must use
pip install -e . --no-build-isolation
which requires that meson, meson-python, ninja, and numpy are
installed in the user's environment.
There is some support for building optional code, but
- FFTW currently requires that it is provided by an external
entity as updating the local build to meson is currently
above my pay grade
The extern/ code has been moved to subprojects/ since it feels
like using the meson subproject support makes sense here.
However, this is just a vibe and I may change my mind. Several
thoughts:
- I haven't removed/cleaned up the existing configure support
- although they are labelled subproject, the build is very
tailored to Sherpa's needs (which may suggest going back to
just make them part of the general build)
- however, this way we can get per-subproject C standard
support, such as wcssubs needing -std=g<v> rather than -std=c<v>
- labelling of the subproject directory: do we want to keep the
version number or not
The editable build now requires the --no-build-isolation flag,
which complicates the build somewhat (as we need to ensure we have
the build components). There are definite improvements when
built like this (I was able to change the code for an extension
module and then the pytest call picked up this change and compiled
just the needed code).
The existing setuptools code has also not been removed.
Initial documentation support has been added, but more is likely
needed.
Version support is incomplete (e.g. the replacement for
sherpa/_version.py and its interactions with the docs build).
There is an assumption that the python3 executable is available,
but this does seem to cause problems with some editable build
changes.
I was getting worried about my understanding of things until I found mesonbuild/meson-python#646 which explained why the "tests of the tests" fail here. So, for now we try to fake the pytest assert and hope it's somewhat similar (at some level any assertion message is "nice to have" but I have been in cases where you get a failure but no information on what actually failed and it is annoying).
This is based on PR sherpa#2220. It tries to do a full - except for XSPEC and DS9 - build and test.
This information can be imputed. But let us not impute!
Contributor
Author
|
Rebased to addres merge conflicts. |
Contributor
Author
|
So, the EDITED TO ADD However, given that these tests run on main/elsewhere, it suggests that the code is not problematic, but maybe the build instructions are? |
Contributor
Author
|
Replaced by #2484 |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Summary
Can we build Sherpa with meson?
Details.
Yes.
Well, kind of.
We can see it building for Linux and macOS (both x86 and arm). It also includes "optional" code such as XSPEC.
BUT it builds and the tests pass. So
Known issues include
pip install . --verbose -Csetup-args=-Dxspec-prefix=$ASCDS_INSTALL -Csetup-args=-Dxspec-libraries=XSFunctions,XSUtil,XS -Csetup-args=-Dbuild-group=false -Csetup-args=-Dbuild-stk=false -Csetup-args=-Dbuild-region=false -Csetup-args=-Dregion-prefix=$ASCDS_INSTALL -Csetup-args=-Dbuild-wcssubs=false -Csetup-args=-Dwcssubs-prefix=$ASCDS_INSTALLversioneerbut is this the best way to do it, and it requires thatpython3is availableexternhas been renamedsubprojectsas this seems like a sensible way to go withmeson, except that these are almost not-really OTS; there's also a question of whether to version-number the directoriestest_tests.pyfail; somehow the assert rewrite thatpytestdoes is not being triggered, possibly because we now use--no-build-isolation;Notes
Editable install success
With an editable install (and
--no-build-isolation) I edited a.ccfile and a test (to use the new compiled module feature) andpytest ..../test...pyactually recompiled just the code I'd changes and ran the test using it. This is noice.libraries
Here's the ldd output for
sherpa/astro/_xspecfrom CIAO 4.17With this build I get
The obvious changes is that the
libXS*.sofiles are not "named"; they just sit there, lurking in the "new" version. There may be others (as I keep on confusing myself).Changes
In the following I use "meson backend" rather blithely, when technically I should say "meson-python backend".
Why?
So, we do not need to do this, but the existing setuptools build is difficult to reason about. As changes have been made to the python packaging standards and tooling, it has been somewhat of a game of whack-a-mole to try to fix issues. Moving to a different build backend (in this case meson) is not guaranteed to be immune from this, but it does give us a chance at a clean slate. There are also some technical improvements - the main one being the improved support for parallel builds, but the editable "no isolation" build is probably better-handled with the meson backend rather than setuptools.
How to build
Python must be available as
python3(it can also be accessible viapython, butpython3is used to handle setting the version in the top-levelmeson.buildfile).Installation
Editable install
You can build without the
--no-build-isolationflat but then I found things likesherpa_testdidn't work, and I am not convinced the "do not rebuild stuff that we don't need rebuilding" works as well).Build
I have managed
but have only done limited testing.
Build configuration
The setuptools approach is to set the build options (essentially, what compiled extensions are to be built and how to do it) are in the
setup.cfgfile. This file is changed by the user before building Sherpa.The meson approach appears to be
meson.optionsfileFor python, this equates to lines like
I don't think there's a way to have the equivalent of
setup.cfg- that is, set the options in a file that gets picked up by the build.The "
-C" option seems to be the way to send these arguments to whatever build tool you are using (although it may depend on the tool). I have been able to sayBuild options
As the configuration system has changed, we have the chance to potentially simplify and standardize the build options. Of course, it's only standardized "up to a point" (that point being the naming of xspec), but let's see.
existing options
I do not think that the
build_sphinxsection is relevant, but I forget exactly what picks this up.proposed options
Changes
I have tried to use the
build-<name>option as the "gate". This is easy forstkandgroupsince webuild it (set to
true)do not build it (set to
false) which coversQUESTION do we need to be more explicit about the difference of "will pick up from CIAO" versus "we do not want". I don't think so since we can not know what is in the user's environment, i.e. we can not explicitly say "there is no stk module". However, there is the next item.
Building the region and wcssubs code is a bit trickier since we can take the code from CIAO, but we still need to build something. If you set the
build-regionorbuild-wcssubstofalseand then do not set the<name>-prefixoption (that is, leave it as the empty string) then the code will not build: this is slightly indirect, but it's the best I could come up with, other than perhaps makingbuild-region/wcssubstake an enumeration rather than a boolean (e.g. something like "build", "nobuild", "buildifyoulike").So, I have made the assumption that we don't need to give separate "install" and "lib" locations when specifying the location of a library; instead you give the "prefix" with the
<libname>-prefixoption and the install/lib locations are assumed to be this prefix + "/install" or + "/lib". This does limit functionality, but I don't think we need to concern ourselves with people who have the install and lib directories in different places.This is particularly noticeable for the XSPEC case, where I just assume that all the related projects (e.g.
ccfitsand the like) are all located in the same place.For the region code, I have avoided the need for explicitly specifying the
ascdmmodule, as this is subsumed into the ungainlyregion-use-cxc-parseroption, and this is the default (since the assumption is that if you are building with an external region library then you are using the CIAO build).For XSPEC we have complications since we need the XSPEC version and HEADAS location, as well as the build libraries. We could separate out "actual XSPEC libraries" - so
XSFunction',XSUtil,XS- from others (e.g.CCfits), but I think for now it's easier just to have one list that gets sent in.hdsp_6.30, we could automate finding them (via a script) so that we could label it ashdsp_Xsay and then this would be found. However, whilst we require the XSPEC version I'm not sure it's worth it. We really need apkg-config-like system for XSPEC, but that's a different conversation!I have simplified the options, since we now try to work out the XSPEC version from the installed library rather than have the user say "use this version". There is an argument to say we should be explicit rather than implicit, but I am not sure we want to be that explicit.