Skip to content
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

Fix building sdist and wheels and conda packages #1232

Merged
merged 8 commits into from
Mar 31, 2022

Conversation

bryanwweber
Copy link
Member

@bryanwweber bryanwweber commented Mar 29, 2022

Changes proposed in this pull request

  • Reduce file size of wheels built on Linux by removing debug symbols
  • Fix writing the sdist so that it is platform-independent
  • Fix tests that write to the local directory
  • Fix the name of the versioned Boost folder in CI
  • Allow instances of pathlib.Path as the filename for write_yaml

  • 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

@bryanwweber bryanwweber marked this pull request as draft March 29, 2022 20:28
We are now downloading 1.78 from jfrog. This has been working up to now
because the cache was available.
YamlWriter.to_file() calls the C++ function after stringify-ing the
filename. This implicitly assumes that filename is a string. With this
change, filename can be either a string or an instance of pathlib.Path.
Tests that write data should do so in the test_work_path to avoid
polluting the user's current working directory and to avoid permission
errors if the current working directory is write-protected.
Some configuration options conflict with creating the sdist, or else are
just unnecessary. These options are checked and set to an appropriate
value if they aren't specified. The build now errors if incompatible
options are explicitly specified.
The config.h file for Cantera needs to have certain values set on the
platform that will build/install Cantera, not the platform building the
sdist. These values are now added to the config.h.in template by
setup.py. Values that are not platform dependent are still handled by
SConscript when the sdist is generated.
Setuptools includes CFLAGS from when Python was built, which may include
the flag to add debug symbols. This dramatically inflates the size of
our wheels, so this change disables those symbols. We also exclude
source files from the built wheel. _cantera.cpp ends up being ~10 MB
uncompressed, so this is a signifcant savings as well. Overall, the
wheel goes from ~100 MB to ~5 MB.
@bryanwweber bryanwweber marked this pull request as ready for review March 30, 2022 02:56
@bryanwweber bryanwweber requested a review from a team March 30, 2022 02:56
speth
speth previously approved these changes Mar 30, 2022
Copy link
Member

@speth speth left a comment

Choose a reason for hiding this comment

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

Looks good to me. I had a couple of questions, but they don't necessarily require any changes.

interfaces/python_sdist/SConscript Outdated Show resolved Hide resolved
interfaces/python_sdist/setup.py Show resolved Hide resolved
Eager resolution of the prefix path to an absolute directory caused the
entire path to the prefix to be used under the stage_dir. As this breaks
the utility of stage_dir, the resolution is conditional on stage_dir
not being among the selected options.

Resolves build failures of the Conda package for Matlab.
@bryanwweber bryanwweber requested a review from speth March 30, 2022 16:13
@bryanwweber bryanwweber changed the title Fix building sdist and wheels Fix building sdist and wheels and conda packages Mar 31, 2022
@bryanwweber
Copy link
Member Author

@speth Since I pushed new commits, this needs another green check, then I think it's good to :shipit:

@speth speth merged commit 6102cd5 into Cantera:main Mar 31, 2022
@bryanwweber bryanwweber deleted the fix-sdist branch August 2, 2024 15:14
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.

2 participants