Skip to content

Do not install C sources in binary distributions#6400

Merged
webknjaz merged 1 commit intoaio-libs:masterfrom
musicinmybrain:c-sources
Dec 9, 2021
Merged

Do not install C sources in binary distributions#6400
webknjaz merged 1 commit intoaio-libs:masterfrom
musicinmybrain:c-sources

Conversation

@musicinmybrain
Copy link
Copy Markdown
Contributor

@musicinmybrain musicinmybrain commented Dec 8, 2021 •
edited by webknjaz
Loading

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
webknjaz
webknjaz reviewed Dec 8, 2021
View reviewed changes
Copy link
Copy Markdown
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.

@codecov
Copy link
Copy Markdown

codecov bot commented Dec 8, 2021 •
edited
Loading

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
Copy Markdown
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.

@musicinmybrain
Do not install C sources in binary distributions
db818a0 
This does not affect source distributions, and Cython sources (.pyx) are
still installed.

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

musicinmybrain commented Dec 8, 2021

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
Copy Markdown
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
Copy Markdown
Contributor

patchback bot commented Dec 9, 2021 •
edited
Loading

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 bot pushed a commit that referenced this pull request Dec 9, 2021
@musicinmybrain @patchback
Do not install C sources in binary distributions (#6400)
8d758e2 
(cherry picked from commit e1ed247)
@patchback patchback bot mentioned this pull request Dec 9, 2021
Merged
5 tasks
@patchback
Copy link
Copy Markdown
Contributor

patchback bot commented Dec 9, 2021 •
edited
Loading

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
@musicinmybrain @patchback
Do not install C sources in binary distributions (#6400)
afbe7a0 
(cherry picked from commit e1ed247)
@patchback patchback bot mentioned this pull request Dec 9, 2021
Merged
5 tasks
webknjaz pushed a commit that referenced this pull request Dec 9, 2021
@patchback @musicinmybrain
[PR #6400/e1ed247d backport][3.8] Do not install C sources in binary …
83e9b8a 
…distributions (#6402)

Co-authored-by: Ben Beasley <[email protected]>
webknjaz pushed a commit that referenced this pull request Dec 9, 2021
@patchback @musicinmybrain
[PR #6400/e1ed247d backport][3.9] Do not install C sources in binary …
97848c0 
…distributions (#6403)

Co-authored-by: Ben Beasley <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@webknjaz webknjaz webknjaz approved these changes

@asvetlov asvetlov Awaiting requested review from asvetlov asvetlov is a code owner

Assignees

No one assigned

Labels

bot:chronographer:provided There is a change note present in this PR

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Binary distributions such as wheels include C sources

2 participants

@musicinmybrain @webknjaz