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

Reduce source distribution size #338

Merged
merged 6 commits into from
Dec 10, 2024
Merged

Reduce source distribution size #338

merged 6 commits into from
Dec 10, 2024

Conversation

ajjackson
Copy link
Collaborator

Depends on #337

meson-python uses git-archive to create the sdist. Some of euphonic's test files are on the large side and we don't need to include them with every copy of the source.

This does have the downside that git-archive can no longer be used to produce a copy of the source with tests, but we weren't using that anyway.

meson-python uses git-archive to create the sdist. Some of euphonic's
test files are on the large side and we don't need to include them
with every copy of the source.

This does have the downside that git-archive can no longer be used
to produce a copy of the source with tests, but we weren't using that
anyway.
@ajjackson ajjackson added this to the v1.4.1 milestone Dec 10, 2024
@ajjackson
Copy link
Collaborator Author

ajjackson commented Dec 10, 2024

Interesting, the tests were running from the sdist!
https://github.com/pace-neutrons/Euphonic/actions/runs/12259358677

Looks like there is more work to do on the wheel-build action, then

Copy link
Contributor

github-actions bot commented Dec 10, 2024

Test Results

    26 files   -   1      26 suites   - 1   1h 39m 49s ⏱️ - 9m 36s
 1 066 tests ±  0   1 060 ✅ ±  0   6 💤 ±0  0 ❌ ±0 
13 333 runs   - 149  13 266 ✅  - 144  67 💤  - 5  0 ❌ ±0 

Results for commit 25475fa. ± Comparison against base commit 6785915.

♻️ This comment has been updated with latest results.

@ajjackson
Copy link
Collaborator Author

ajjackson commented Dec 10, 2024

Some very puzzling stuff going on here: the wheel tests are failing only on Linux, where it claims to be unable to find the run_tests.py. The file is clearly visible from ls -R in the previous workflow step, and is accessed via an absolute path.

Screenshot 2024-12-10 at 16 28 00
Screenshot 2024-12-10 at 16 28 54

Maybe abs paths don't work well here for some reason?
@ajjackson
Copy link
Collaborator Author

@oerc0122

Ahhh... I see the problem. The linux builds are running in a (manylinux) container, so they don't have access to the filesystem.

In that case, I think the options are

a) build Linux (and other platforms?) from the git checkout and not the sdist; or
b) create our own isolated wheel-testing workflow step that uses the wheel as input.

b) offers a tiny bit more purity for a lot of extra work. Let's do a)

While the purity of building from sdist was appealing, it is difficult
for cibuildwheel to cleanly run an external test suite as it
containerizes the Linux build (to deal with ManyLinux requirements).

We are still testing the sdist (on Windows) so just have to trust in
cibuildwheel's own efforts to separate the testing and source from the
installation. (Which it does seem to work quite hard at.)
@ajjackson ajjackson marked this pull request as ready for review December 10, 2024 17:25
Copy link

codecov bot commented Dec 10, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Please upload report for BASE (test_windows_sdist@6785915). Learn more about missing BASE report.

Additional details and impacted files
@@                  Coverage Diff                  @@
##             test_windows_sdist     #338   +/-   ##
=====================================================
  Coverage                      ?   95.43%           
=====================================================
  Files                         ?       29           
  Lines                         ?     4209           
  Branches                      ?      641           
=====================================================
  Hits                          ?     4017           
  Misses                        ?      114           
  Partials                      ?       78           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@ajjackson
Copy link
Collaborator Author

All green, finally 😮‍💨

https://github.com/pace-neutrons/Euphonic/actions/runs/12261330267

The sdist is still almost 10x the wheel size at 1MB. Probably due to docs/images; may as well strip those out as well?

@ajjackson ajjackson merged commit be38a32 into test_windows_sdist Dec 10, 2024
27 checks passed
@ajjackson ajjackson deleted the package-size branch December 10, 2024 17:52
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