Fix build issues - macOS rpath, wheel paths, missing pytest #1229

speth · 2022-03-26T23:54:23Z

Changes proposed in this pull request

Fix SCons always rebuilding wheels due to not predicting the file name correctly -- I ended up adopting @bryanwweber's approach for setting MACOSX_DEPLOYMENT_TARGET from Add package workflow #1158 after all
Force SCons to include rpath flags when linking on macOS so libraries that are not in system locations can be used for dependencies (such as libraries installed in a conda environment)
Provide a better error message in the test summary when pytest is not installed
Suppress XML deprecation warnings from ctml_writer that are generated during scons build

Checklist

The pull request includes a clear description of this code change
Commit messages have short titles and reference relevant issues
Build passes (scons build & scons test) and unit tests address code coverage
Style & formatting of contributed code follows contributing guidelines
The pull request is ready for review

Failing tests

bryanwweber · 2022-03-27T00:07:50Z

Looks like the docs need a pinned jinja2, like the website did

bryanwweber · 2022-03-27T00:15:03Z

The Windows failures are due to Setuptools. Something with Python 3.7 apparently. I'll look when I'm at a computer.

codecov · 2022-03-27T00:16:23Z

Codecov Report

Merging #1229 (5d4d87c) into main (04142f6) will not change coverage.
The diff coverage is 66.66%.

❗ Current head 5d4d87c differs from pull request most recent head c00ded9. Consider uploading reports for the commit c00ded9 to get more accurate results

@@           Coverage Diff           @@
##             main    #1229   +/-   ##
=======================================
  Coverage   65.47%   65.47%           
=======================================
  Files         327      327           
  Lines       46350    46350           
  Branches    19688    19688           
=======================================
  Hits        30349    30349           
  Misses      13476    13476           
  Partials     2525     2525

Impacted Files	Coverage Δ
src/clib/Cabinet.h	`19.60% <ø> (ø)`
src/clib/ct.cpp	`7.39% <ø> (ø)`
src/clib/ctmultiphase.cpp	`0.00% <0.00%> (ø)`
src/clib/ctonedim.cpp	`0.00% <ø> (ø)`
src/clib/ctreactor.cpp	`6.54% <ø> (ø)`
src/clib/ctrpath.cpp	`0.00% <ø> (ø)`
src/clib/ctsurf.cpp	`0.00% <ø> (ø)`
include/cantera/kinetics/MultiRate.h	`84.48% <100.00%> (ø)`

📣 Codecov can now indicate which changes are the most critical in Pull Requests. Learn more

speth · 2022-03-28T00:47:23Z

I'm very confused by the error building the Python module in those two instances (which I don't think has anything to do with the changes introduced in this PR, given main is also failing). Searching turned up a couple of other instances reporting failures at the same place inside distutils, but no resolution. For example: tensorflow/tensorflow#27477 and jwyang/faster-rcnn.pytorch#681.

For posterity, the traceback in the build log is:

  running build_ext
  building 'cantera._cantera' extension
  Traceback (most recent call last):
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 363, in <module>
      main()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 345, in main
      json_out['return_val'] = hook(**hook_input['kwargs'])
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 262, in build_wheel
      metadata_directory)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\build_meta.py", line 245, in build_wheel
      wheel_directory, config_settings)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\build_meta.py", line 229, in _build_with_temp_dir
      self.run_setup()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\build_meta.py", line 174, in run_setup
      exec(compile(code, __file__, 'exec'), locals())
    File "setup.py", line 5, in <module>
      setup(ext_modules=[extension])
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\__init__.py", line 79, in setup
      return distutils.core.setup(**attrs)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\core.py", line 148, in setup
      return run_commands(dist)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\core.py", line 163, in run_commands
      dist.run_commands()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\dist.py", line 967, in run_commands
      self.run_command(cmd)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\dist.py", line 1196, in run_command
      super().run_command(command)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\dist.py", line 986, in run_command
      cmd_obj.run()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\wheel\bdist_wheel.py", line 299, in run
      self.run_command('build')
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\cmd.py", line 313, in run_command
      self.distribution.run_command(command)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\dist.py", line 1196, in run_command
      super().run_command(command)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\dist.py", line 986, in run_command
      cmd_obj.run()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\command\build.py", line 135, in run
      self.run_command(cmd_name)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\cmd.py", line 313, in run_command
      self.distribution.run_command(command)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\dist.py", line 1196, in run_command
      super().run_command(command)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\dist.py", line 986, in run_command
      cmd_obj.run()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\command\build_ext.py", line 79, in run
      _build_ext.run(self)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\Cython\Distutils\old_build_ext.py", line 186, in run
      _build_ext.build_ext.run(self)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\command\build_ext.py", line 339, in run
      self.build_extensions()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\Cython\Distutils\old_build_ext.py", line 195, in build_extensions
      _build_ext.build_ext.build_extensions(self)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\command\build_ext.py", line 448, in build_extensions
      self._build_extensions_serial()
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\command\build_ext.py", line 473, in _build_extensions_serial
      self.build_extension(ext)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\command\build_ext.py", line 202, in build_extension
      _build_ext.build_extension(self, ext)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\command\build_ext.py", line 559, in build_extension
      target_lang=language)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\ccompiler.py", line 717, in link_shared_object
      extra_preargs, extra_postargs, build_temp, target_lang)
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\setuptools\_distutils\_msvccompiler.py", line 483, in link
      build_temp = os.path.dirname(objects[0])
  IndexError: list index out of range
  [end of output]
  
  note: This error originates from a subprocess, and is likely not a problem with pip.
  ERROR: Failed building wheel for Cantera
Failed to build Cantera
ERROR: Failed to build one or more wheels
scons: *** [build\python\dist\Cantera-2.6.0b1-cp37-cp37-win_amd64.whl] Error 1

speth · 2022-03-29T01:03:45Z

Investigating the more detailed logging output that has been temporarily enabled, the difference that I've noticed between these two cases is that when the build succeeds, the logs for pip wheel contain (in part):

  ...
  creating build\lib.win-amd64-3.9\cantera\examples\thermo
  creating build\lib.win-amd64-3.9\cantera\examples\transport
  running build_ext
  installing to build\bdist.win-amd64\wheel
  running install
  running install_lib
  ...

while for the failing builds, they contain the following:

  creating build\lib.win-amd64-3.7\cantera\examples\thermo
  creating build\lib.win-amd64-3.7\cantera\examples\transport
  running build_ext
  building 'cantera._cantera' extension
  Traceback (most recent call last):
    File "C:\hostedtoolcache\windows\Python\3.7.9\x64\lib\site-packages\pip\_vendor\pep517\in_process\_in_process.py", line 363, in <module>
      main()

which indicates that in the failing case, setuptools or wheel or pip (I really don't understand any of this infrastructure) erroneously thinks there's actually something that needs to be compiled.

speth · 2022-03-29T12:56:14Z

Looking through earlier builds on the main branch, the failing builds with Python 3.7 are due to changing from setuptools 60.x to 61.x. Pinning setuptools to version 60 on those builders seems to fix the problem.

speth · 2022-03-29T13:11:45Z

The release notes for setuptools 61.0.0 mentions pypa/setuptools#2894 as a "breaking change"

Projects that currently don’t specify both packages and py_modules in their configuration and contain extra folders or Python files (not meant for distribution), might see these files being included in the wheel archive or even experience the build to fail.

If this is the cause of our problems, I have no idea why it's restricted to such a narrow configuration of builders. Also, I find it irritating that there is no logging information even at the highest debug loglevel about what setuptools is trying to do during the build_ext step.

Eliminate duplication in building the list of external libraries to link to between SConstruct and src/SConscript.

On macOS, sysconfig.get_platform() returns a string like 'macosx-11.0-arm64', but the wheel filename will actually contain 'macosx_11_0_arm64'.

This prevents rebuilds of the wheel file every time 'scons build' is run because the actual wheel file doesn't match the expected name. Co-authored-by: Bryan W. Weber <bryan.w.weber@gmail.com>

ischoegl

Don't have much to add here.

interfaces/cython/setup-debian.py

bryanwweber · 2022-03-29T22:42:19Z

Thanks for all the work here @speth! I'll review it tonight. My only concern right away is that the wheel has the right name, it should have some platform information in it.

speth · 2022-03-29T23:55:11Z

Huh, I didn't realize that the bits in setup.py were what was responsible for tricking setuptools into generating a platform-specific wheels. Let's see if overriding setuptools.dist.Distribution.has_ext_modules works any better...

bryanwweber · 2022-03-30T00:00:30Z

Huh, I didn't realize that the bits in setup.py were what was responsible for tricking setuptools into generating a platform-specific wheels.

It's not a trick, really... C extensions are what cause the requirement for platform-specific wheels, so you have to tell setuptools that it's building an extension. Previously, it decided that since there were no source files, it could just package the library. Dunno why that stopped working.

speth · 2022-03-30T00:26:30Z

Yes, I understand that the compiled extension is the reason that the wheel should be platform specific. But what was being specified was an empty extension module, while we just stick the actual compiled extension module in as if it's just "data" and setuptools does not recognize it as creating a platform dependency (which would be asking a lot). So I think this "empty extension module" really is a bit of a hack and setuptools ought to provide a better way of handling situations where there is platform-specific package content but it's not being compiled by setuptools.

bryanwweber

Thanks @speth! The tests pass and I can confirm that I'm getting platform specific wheels locally. Just a couple minor comments here.

bryanwweber · 2022-03-30T00:53:22Z

interfaces/cython/setup.cfg.in

@@ -49,6 +49,7 @@ packages =
    cantera.test
    cantera.test.data
    cantera.examples
+py_modules =


I don't think this line is required. At least, the same files are packaged in the wheel whether this is here or not.

This was based on reading of the (conflicting) documentation about this new "discovery" feature of setuptools, where it says:

Projects that currently don’t specify both packages and py_modules in their configuration and contain extra folders or Python files (not meant for distribution), might see these files being included in the wheel archive or even experience the build to fail.

Elsewhere, it says:

Automatic discovery will only be enabled if you don’t provide any configuration for packages and py_modules. If at least one of them is explicitly set, automatic discovery will not take place.

So I really don't know. But I guess we can see if everything still works without it.

I have setuptools 61.1.1, so I'm not sure.

Their documentation is terrible, a mishmash of stuff copied from the original distutils (and still duplicated in the official Python docs), random pages of (maybe?) best practices, and no complete API specification. But hey, it looks pretty!

bryanwweber · 2022-03-30T00:56:05Z

test/python/runCythonTests.py

        print("\n* ERROR: The Cantera Python test suite requires "
            "the Python package 'pytest'.")
        print("* ERROR: Use pip or conda to install 'pytest', "
            "which will enable this feature.")
-        sys.exit(1)
+        sys.exit(21)


Was this change intentional?

Yes, this specific error code now checked for in test/SConscript so we can provide a better error message in the case where pytest is not installed rather than just "the entire Python test suite failed". I'll add a comment for this.

Something changed in setuptools that causes the old method of forcing platform-specific wheels to be built to break for certain configurations (specifically, Windows builds with Python 3.7). This alternative approach appears to be more robust. Also increase the logging from 'pip wheel' to help with debugging

The old way of handling declarations for static members of a template class was weird and required different implementations for different compilers. This approach is standards-conformant and works everywhere.

bryanwweber

Thanks again @speth, let's

bryanwweber · 2022-03-30T02:41:13Z

interfaces/cython/setup.py

+class BinaryDistribution(Distribution):
+    def has_ext_modules(self):
+        return True


This is my new favorite Python class ❤️

speth added macOS compiling labels Mar 26, 2022

ischoegl previously approved these changes Mar 27, 2022

View reviewed changes

speth mentioned this pull request Mar 27, 2022

Document compilation in conda environment Cantera/cantera-website#181

Closed

anthony-walker mentioned this pull request Mar 28, 2022

Adding adaptive preconditioner functionality to Cantera #1010

Merged

4 tasks

speth force-pushed the fix-build-issues branch 2 times, most recently from 872a8cd to ec3f8d3 Compare March 28, 2022 20:55

speth force-pushed the fix-build-issues branch 3 times, most recently from 8d51aeb to 085daf3 Compare March 29, 2022 03:17

speth marked this pull request as draft March 29, 2022 03:18

speth force-pushed the fix-build-issues branch from 085daf3 to 004d528 Compare March 29, 2022 13:00

speth marked this pull request as ready for review March 29, 2022 13:01

speth marked this pull request as draft March 29, 2022 14:56

speth and others added 4 commits March 29, 2022 16:29

[SCons] Fix linking to system version of fmt

27e4359

Eliminate duplication in building the list of external libraries to link to between SConstruct and src/SConscript.

[SCons] Fix expected name of .whl file

dfb4caa

On macOS, sysconfig.get_platform() returns a string like 'macosx-11.0-arm64', but the wheel filename will actually contain 'macosx_11_0_arm64'.

[SCons] Determine name of macOS wheel in advance

21e95e9

This prevents rebuilds of the wheel file every time 'scons build' is run because the actual wheel file doesn't match the expected name. Co-authored-by: Bryan W. Weber <bryan.w.weber@gmail.com>

[SCons] Fix RPATH linkage on macOS

bace398

speth force-pushed the fix-build-issues branch from c423904 to 0525d09 Compare March 29, 2022 20:34

speth marked this pull request as ready for review March 29, 2022 21:12

speth requested a review from a team March 29, 2022 21:15

ischoegl reviewed Mar 29, 2022

View reviewed changes

interfaces/cython/setup-debian.py Outdated Show resolved Hide resolved

speth force-pushed the fix-build-issues branch from 0525d09 to 724df14 Compare March 29, 2022 21:54

speth force-pushed the fix-build-issues branch from f0cbe6f to 5d4d87c Compare March 30, 2022 00:05

bryanwweber previously approved these changes Mar 30, 2022

View reviewed changes

speth added 6 commits March 29, 2022 21:33

[SCons] Provide better error message when pytest is missing

f7b7255

[CI] Pin jinja2 to fix incompatibility with Sphinx<4.0

e119193

Fix MSVC warning about mismatched qualifiers

9885508

Suppress ctml_writer deprecation warnings during build

142107f

Eliminate NEEDS_GENERIC_TEMPL_STATIC_DECL

c00ded9

The old way of handling declarations for static members of a template class was weird and required different implementations for different compilers. This approach is standards-conformant and works everywhere.

speth dismissed bryanwweber’s stale review via c00ded9 March 30, 2022 01:34

speth force-pushed the fix-build-issues branch from 5d4d87c to c00ded9 Compare March 30, 2022 01:34

bryanwweber approved these changes Mar 30, 2022

View reviewed changes

bryanwweber merged commit 60d56e1 into Cantera:main Mar 30, 2022

speth deleted the fix-build-issues branch March 30, 2022 02:53

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix build issues - macOS rpath, wheel paths, missing pytest #1229

Fix build issues - macOS rpath, wheel paths, missing pytest #1229

speth commented Mar 26, 2022 •

edited

Loading

bryanwweber commented Mar 27, 2022

bryanwweber commented Mar 27, 2022

codecov bot commented Mar 27, 2022 •

edited

Loading

speth commented Mar 28, 2022 •

edited

Loading

speth commented Mar 29, 2022

speth commented Mar 29, 2022

speth commented Mar 29, 2022

ischoegl left a comment

bryanwweber commented Mar 29, 2022

speth commented Mar 29, 2022

bryanwweber commented Mar 30, 2022

speth commented Mar 30, 2022

bryanwweber left a comment

bryanwweber Mar 30, 2022

speth Mar 30, 2022

bryanwweber Mar 30, 2022

bryanwweber Mar 30, 2022

speth Mar 30, 2022

bryanwweber left a comment

bryanwweber Mar 30, 2022

Fix build issues - macOS rpath, wheel paths, missing pytest #1229

Fix build issues - macOS rpath, wheel paths, missing pytest #1229

Conversation

speth commented Mar 26, 2022 • edited Loading

bryanwweber commented Mar 27, 2022

bryanwweber commented Mar 27, 2022

codecov bot commented Mar 27, 2022 • edited Loading

Codecov Report

speth commented Mar 28, 2022 • edited Loading

speth commented Mar 29, 2022

speth commented Mar 29, 2022

speth commented Mar 29, 2022

ischoegl left a comment

Choose a reason for hiding this comment

bryanwweber commented Mar 29, 2022

speth commented Mar 29, 2022

bryanwweber commented Mar 30, 2022

speth commented Mar 30, 2022

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Mar 30, 2022

Choose a reason for hiding this comment

speth Mar 30, 2022

Choose a reason for hiding this comment

bryanwweber Mar 30, 2022

Choose a reason for hiding this comment

bryanwweber Mar 30, 2022

Choose a reason for hiding this comment

speth Mar 30, 2022

Choose a reason for hiding this comment

bryanwweber left a comment

Choose a reason for hiding this comment

bryanwweber Mar 30, 2022

Choose a reason for hiding this comment

speth commented Mar 26, 2022 •

edited

Loading

codecov bot commented Mar 27, 2022 •

edited

Loading

speth commented Mar 28, 2022 •

edited

Loading