-
-
Notifications
You must be signed in to change notification settings - Fork 346
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
Add package workflow #1158
Add package workflow #1158
Conversation
f297b72
to
d11225c
Compare
@Cantera/committers I'd appreciate some eyes on this PR, as I'm getting into rewriting the Python installer logic in the SConscript. I added it to this branch because I'm hoping to use the rewritten Python installer to fix the Conda packages, and then add a job here to run the Conda builds when a PR is merged to See the details section 👇 for some justification/explanation of the changes: Click for more
FYI @speth, as we discussed in Cantera/enhancements#129, this branch has an example of triggering a workflow in another repo. You made a good point that it'd be nice if we can somehow drag the status of the triggered workflow back into the status of this repo. I'm sure it's possible, I'll see how to do it. Worst case, it will be a "badge" on the README with the workflow status in the other repositories. |
👀 ... (kidding) I am aware of what you pointed out about (3) ... have been using the Triggering Conda builds upon PR merges sounds terrific! 🎉 ... this comes very close to Cantera/enhancements#24 (which is superseded by Cantera/enhancements#37) and should also eliminate issues like Cantera/conda-recipes#24 (haven't checked recently whether this has since been resolved). |
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
@speth / @ischoegl OK, after reflecting this week, I think enforcing installation directories is probably a step too far. The justification for it isn't quite as strong as I thought, and you've both raised some really good points. In the end, I think I'll defer improved error messages from this PR to a later one, and just focus this one on using pip as the Python installer so the Conda packages can work again. I'm going to mark a bunch of the comments in this thread as off topic to reduce the clutter as this PR moves towards review. |
c624ce5
to
5a19786
Compare
a06a88f
to
9e7a48a
Compare
Since Python builds are done in the build directory, we don't need to clean up the interfaces folder anymore.
We're dropping Python 3.6, Windows builds need wheel installed, and the GitHub Actions macOS runners are notoriously slow.
The Sourceforge "pilotfibre" domain has an expired SSL certificate and doesn't seem to be maintained anymore.
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 admit that I understand approximately nothing about how Python packaging is supposed to work anymore, but I don't see any major problems with this, and it seems to work correctly for the couple of different configurations I tried.
SConstruct
Outdated
if env["python_package"] == "full" and env["OS"] == "Darwin": | ||
# sysconfig.get_platform() looks something like: macosx-11.0-arm64 | ||
# MACOSX_DEPLOYMENT_TARGET should be the same as or lower than | ||
# the one set when Python was compiled to ensure ABI compatibility. | ||
# If it's been set in the user's environment and passed through env_vars | ||
# configuration, use that value instead. | ||
if not env["ENV"].get("MACOSX_DEPLOYMENT_TARGET", False): | ||
info = get_command_output( | ||
env["python_cmd"], | ||
"-c", | ||
"import sysconfig; print(sysconfig.get_platform())" | ||
) | ||
env["ENV"]["MACOSX_DEPLOYMENT_TARGET"] = info.split("-")[1] | ||
|
||
env.Append( | ||
CXXFLAGS=f"-mmacosx-version-min={env['ENV']['MACOSX_DEPLOYMENT_TARGET']}" | ||
) | ||
|
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.
First, I think if you define the MACOSX_DEPLOYMENT_TARGET
environment variable, you don't need to specify the compiler option. If you want to use the compiler option instead, though, it should also be set for CCFLAGS
so it will affect pure C code like SUNDIALS.
Second, while I'll admit this is as clear as mud to me, I'm not sure it's necessary for libraries that get linked together to have the same OS target version. The conda-forge maintainer docs do have a bit to say about this (https://conda-forge.org/docs/maintainer/knowledge_base.html#requiring-newer-macos-sdks), which mostly boils down to saying that they compile for 10.9 to maintain compatibility with macOS 10.9, but suggests that requiring a higher version is something that can be done.
Certainly, when building packages that we distribute, we should be setting MACOSX_DEPLOYMENT_TARGET to an older value than whatever it defaults to, and I think that should be done regardless of whether you're compiling the Python module (but again, I don't think anything needs to be written here to achieve that).
SConscript('interfaces/python_minimal/SConscript') | ||
if env["python_package"] == "full": | ||
VariantDir("build/python", "interfaces/cython", duplicate=True) | ||
SConscript("build/python/SConscript") |
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 was sort of hoping we could move toward doing away with any copying of the Python module parts, since that would make it easier to do rapid iteration and interactive debugging of the pure-Python components.
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.
That's unfortunate... For me, it's so much easier to reason about actual files than abstract file nodes. I think it also streamlines the installation method (pip), which doesn't have to account for the built library being in a different location, and which also doesn't know anything about file nodes.
When possible, SCons hard links files rather than copying them. However, most editors (to my understanding) use a copy-then-move strategy to save files, which creates a new inode and breaks the hardlink, so that doesn't actually help us.
What I've done is use pytest-watch
, which allows automatically running a command before running the test suite whenever a file is saved. This doesn't support the interactive feature development case so much, but it's better than nothing.
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.
@speth Do you have any further thoughts here?
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 gave this a spin on both Linux (conda on Fedora) and Windows (likewise from within conda). Once I made sure to run scons clean
, things worked as expected 😆
For windows conda with prefix (i.e. scons build -j4 prefix=C:\Users\ischo\cantera2.6.0a4
, and scons install
), I get
[...]
postInstallMessage(["finish_install"], [])
Cantera has been successfully installed.
File locations:
library files C:\Users\ischo\cantera2.6.0a4\lib
C++ headers C:\Users\ischo\cantera2.6.0a4\include
samples C:\Users\ischo\cantera2.6.0a4\samples
data files C:\Users\ischo\cantera2.6.0a4\data
input file converters C:\Users\ischo\miniconda3\envs\cantera-dev\Scripts
Python package C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages
Python samples C:\Users\ischo\miniconda3\envs\cantera-dev\Lib\site-packages\cantera\examples
scons: done building targets.
so the input file converter location is now displayed correctly. While it's not what I would have expected, it works and is user-friendly. As such, this closes #1192.
Uninstalling works in all cases (again, scons clean
in between compilations is pretty important).
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.
While I lack the background to really comment on the packaging aspect, I ran a couple of tests installing and everything works as expected. The install message newline and script location when prefix
is specified is optional (presumably, the script location is dictated by python_prefix
?).
If the prefix is set, the Python package should be installed there.
@ischoegl Thanks, I think I addressed your comments! It also helped me find the problem with the |
Codecov Report
@@ Coverage Diff @@
## main #1158 +/- ##
=======================================
Coverage 65.43% 65.44%
=======================================
Files 320 327 +7
Lines 46249 46321 +72
Branches 19657 19688 +31
=======================================
+ Hits 30265 30314 +49
- Misses 13454 13475 +21
- Partials 2530 2532 +2
📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more |
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.
Thanks! Tried on windows with prefix
again, but something apparently broke:
(cantera-dev) PS C:\Users\ischo\GitHub\cantera> scons build -j4 prefix=C:\Users\ischo\cantera2.6.0a4
scons: Reading SConscript files ...
INFO: SCons is using the following Python interpreter: C:\Users\ischo\miniconda3\envs\cantera-dev\python.exe
Compiling with MSVC 14.3
Compiling for architecture: amd64
Compiling using the following toolchain(s): ['default']
INFO: Building Cantera from git commit '8e1079a21'
INFO: Configuration variables read from 'cantera.conf' and command line:
prefix = 'C:\\Users\\ischo\\cantera2.6.0a4'
INFO: Adding conda include and library paths: C:\Users\ischo\miniconda3\envs\cantera-dev
Checking for C++ header file cmath... yes
Checking for C++ header file fmt/ostream.h... no
INFO: Using private installation of fmt library.
INFO: Found fmt version 6.2.1
Checking for YAML::Node().Mark()... yes
INFO: Using system installation of yaml-cpp library.
Checking for C++ header file gtest/gtest.h... no
Checking for C++ header file gmock/gmock.h... no
INFO: Using Googletest from Git submodule
Checking for C++ header file eigen3/Eigen/Dense... yes
INFO: Using system installation of Eigen.
INFO: Found Eigen version 3.4.0
Checking whether __GLIBCXX__ is declared... no
Checking whether _LIBCPP_VERSION is declared... no
Checking whether __clang__ is declared... no
Checking for C++ library iomp5... no
Checking for C++ library omp... no
Checking for C++ library gomp... no
INFO: Found Boost version 1.77
Checking whether boost::core::demangle is declared... yes
Checking for CVodeCreate(CV_BDF, CV_NEWTON) in C++ library sundials_cvodes... no
Checking for CVodeCreate(CV_BDF) in C++ library sundials_cvodes... no
Checking for SUNContext ctx; SUNContext_Create(0, &ctx) in C++ library sundials_cvodes... no
Checking for double x; log(x) in C library None... yes
INFO: Using private installation of Sundials version 5.3.
INFO: Skipping compilation of the Fortran 90 interface.
INFO: Using NumPy version 1.22.2.
INFO: Using Cython version 0.29.28.
INFO: Building the full Python package for Python 3.10
File "<string>", line 10
prefix="C:\Users\ischo\cantera2.6.0a4")
^
SyntaxError: (unicode error) 'unicodeescape' codec can't decode bytes in position 2-3: truncated \UXXXXXXXX escape
CalledProcessError: Command '['C:\\Users\\ischo\\miniconda3\\envs\\cantera-dev\\python.exe', '-c', '\nfrom pip import __version__ as pip_version\nfrom pkg_resources import parse_version\nimport pip\nimport json\npip_version = parse_version(pip_version)\nif pip_version < parse_version("10.0.0"):\n from pip.locations import distutils_scheme\n scheme = distutils_scheme("Cantera", user=False, root=None,\n prefix="C:\\Users\\ischo\\cantera2.6.0a4")\nelse:\n from pip._internal.locations import get_scheme\n scheme = get_scheme("Cantera", user=False, root=None,\n prefix="C:\\Users\\ischo\\cantera2.6.0a4")\n\nif not isinstance(scheme, dict):\n scheme = {k: getattr(scheme, k) for k in dir(scheme)\n if not k.startswith("_")}\nprint(json.dumps(scheme))\n']' returned non-zero exit status 1.:
File "C:\Users\ischo\GitHub\cantera\SConstruct", line 1964:
SConscript("build/python/SConscript")
File "C:\Users\ischo\miniconda3\envs\cantera-dev\lib\site-packages\SCons\Script\SConscript.py", line 660:
return method(*args, **kw)
File "C:\Users\ischo\miniconda3\envs\cantera-dev\lib\site-packages\SCons\Script\SConscript.py", line 597:
return _SConscript(self.fs, *files, **subst_kw)
File "C:\Users\ischo\miniconda3\envs\cantera-dev\lib\site-packages\SCons\Script\SConscript.py", line 285:
exec(compile(scriptdata, scriptname, 'exec'), call_stack[-1].globals)
File "C:\Users\ischo\GitHub\cantera\build\python\SConscript", line 154:
install_locs = get_pip_install_location(localenv["python_cmd"], user_install,
File "C:\Users\ischo\GitHub\cantera\site_scons\buildutils.py", line 1245:
return json.loads(get_command_output(python_cmd, "-c", install_script))
File "C:\Users\ischo\GitHub\cantera\site_scons\buildutils.py", line 1199:
data = subprocess.run(
File "C:\Users\ischo\miniconda3\envs\cantera-dev\lib\subprocess.py", line 524:
raise CalledProcessError(retcode, process.args,
(cantera-dev) PS C:\Users\ischo\GitHub\cantera>
(I made sure to run scons clean
)
Yeah, this is the same failure as on the Windows CI. Won't be hard to fix 👍 |
@ischoegl Ah, this is apparently a different error, actually, due to Windows insistence on using the wrong slash for a path separator. The paths should be normalized by that point, so I'm not sure what's going on, but I can reproduce locally and I'll fix it 👍 |
The default_prefix variable needs to always be set. This also fixes backslash-separated paths by replacing with forward slash. Although this is most likely to occur on Windows, it's not platform specific.
On Windows, normpath turns forward slashes into backslashes. We want to avoid that because paths on Windows that have backslash turn into escapes that break things.
Hopefully move this after all path manipulations happen.
post-install message for
|
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.
While the forward/backward slashes aren't pretty, this shouldn't hold back the beta release.
add-package-workflow was a temporary override and that branch has been merged. xref Cantera/cantera#1158
Changes proposed in this pull request
main
in this repo and to tags pushed to this repo.pip
as the builder and installerChecklist
scons build
&scons test
) and unit tests address code coverage