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

Do not install C sources in binary distributions #6400

Merged
merged 1 commit into from
Dec 9, 2021

Conversation

musicinmybrain
Copy link
Contributor

@musicinmybrain musicinmybrain commented Dec 8, 2021

This does not affect source distributions, and Cython sources (.pyx) are
still installed.

What do these changes do?

Prevent C sources (_find_header.c, _find_header.h, _helpers.c, _http_parser.c, _http_writer.c, and _websocket.c) from being included in binary distributions such as wheels, without affecting Cython sources (_cparser.pxd, _find_header.pxd, _headers.pxi, _helpers.pyx, _http_parser.pyx, _http_writer.pyx, and _websocket.pyx) or source distributions.

Are there changes in behavior for the user?

Nothing changes except that the binary wheels are much smaller because they do not contain ~1.7MB of C sources.

Related issue number

Resolves #6399.

Checklist

  • I think the code is well written
  • Unit tests for the changes exist This doesn’t seem necessary.
  • Documentation reflects the changes No changes seem necessary.
  • If you provide code modification, please add yourself to CONTRIBUTORS.txt
    • The format is <Name> <Surname>.
    • Please keep alphabetical order, the file is sorted by names.
    • This contribution is trivial, so this doesn’t seem necessary, but I can add it if desired.
  • Add a new news fragment into the CHANGES folder
    • name it <issue_id>.<type> for example (588.bugfix)
    • if you don't have an issue_id change it to the pr id after creating the pr
    • ensure type is one of the following:
      • .feature: Signifying a new feature.
      • .bugfix: Signifying a bug fix.
      • .doc: Signifying a documentation improvement.
      • .removal: Signifying a deprecation or removal of public API.
      • .misc: A ticket has been closed, but it is not of interest to users.
    • Make sure to use full sentences with correct case and punctuation, for example: "Fix issue with non-ascii contents in doctest text files."

@psf-chronographer psf-chronographer bot added the bot:chronographer:provided There is a change note present in this PR label Dec 8, 2021
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

Not sure how I feel about this in general but here's some actionable feedback for now.

setup.cfg Outdated Show resolved Hide resolved
CHANGES/6399.bugfix Outdated Show resolved Hide resolved
@codecov
Copy link

codecov bot commented Dec 8, 2021

Codecov Report

Merging #6400 (db818a0) into master (dcb44a9) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #6400   +/-   ##
=======================================
  Coverage   93.34%   93.34%           
=======================================
  Files         104      104           
  Lines       30456    30456           
  Branches     3064     3064           
=======================================
  Hits        28429    28429           
  Misses       1850     1850           
  Partials      177      177           
Flag Coverage Δ
unit 93.26% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.


Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update dcb44a9...db818a0. Read the comment docs.

@webknjaz
Copy link
Member

webknjaz commented Dec 8, 2021

Not sure how I feel about this in general but here's some actionable feedback for now.

Based on Andrew's comments in the issue, this will be good to go once those comments are addressed.

This does not affect source distributions, and Cython sources (.pyx) are
still installed.

Fixes aio-libs#6399.
@musicinmybrain
Copy link
Contributor Author

https://github.com/aio-libs/aiohttp/runs/4458544871?check_suite_focus=true#step:10:2941

E ResourceWarning: unclosed transport <_SelectorSocketTransport closing fd=46>

Doesn’t look like the CI failure has anything to do with the PR.

@webknjaz webknjaz enabled auto-merge (squash) December 9, 2021 02:02
@webknjaz
Copy link
Member

webknjaz commented Dec 9, 2021

Doesn’t look like the CI failure has anything to do with the PR.

That's probably a race condition in tests. It's not related to the CI itself but the tests could be improved, for sure. And of course, it's a separate effort/PR. I'm restarting the CI for now.

@webknjaz webknjaz merged commit e1ed247 into aio-libs:master Dec 9, 2021
@patchback
Copy link
Contributor

patchback bot commented Dec 9, 2021

Backport to 3.8: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.8/e1ed247dd3aa9a2fc3cd6d87b4845849da6ee494/pr-6400

Backported as #6402

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

@patchback
Copy link
Contributor

patchback bot commented Dec 9, 2021

Backport to 3.9: 💚 backport PR created

✅ Backport PR branch: patchback/backports/3.9/e1ed247dd3aa9a2fc3cd6d87b4845849da6ee494/pr-6400

Backported as #6403

🤖 @patchback
I'm built with octomachinery and
my source is open — https://github.com/sanitizers/patchback-github-app.

patchback bot pushed a commit that referenced this pull request Dec 9, 2021
webknjaz pushed a commit that referenced this pull request Dec 9, 2021
webknjaz pushed a commit that referenced this pull request Dec 9, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bot:chronographer:provided There is a change note present in this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Binary distributions such as wheels include C sources
2 participants