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.

Sign up for GitHub

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

Jump to bottom

build: avoid using CMP for BZ2File #31198

Closed
wants to merge 2 commits into from
Closed

build: avoid using CMP for BZ2File #31198

wants to merge 2 commits into from

Conversation

ghost
Copy link

@ghost ghost commented Jan 5, 2020

Some Python distributions do not support
context manager protocol (CMP) for BZ2File.

Fixes #30949

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • commit message follows commit guidelines

build: avoid using CMP for BZ2File
6547682 
Some Python distributions do not support
context manager protocol (CMP) for BZ2File.
@nodejs-github-bot nodejs-github-bot added the build Issues and PRs related to build files or the CI. label Jan 5, 2020
bnoordhuis
bnoordhuis approved these changes Jan 6, 2020
View reviewed changes
configure.py Outdated Show resolved Hide resolved
@BridgeAR BridgeAR added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Jan 6, 2020
@BridgeAR
Copy link
Member

BridgeAR commented Jan 6, 2020

Marked as author ready. I do not think we have any CI that could test this, so we should not require a CI run.

@Trott
Copy link
Member

Trott commented Jan 7, 2020

/ping @nodejs/python

@Trott
Copy link
Member

Trott commented Jan 7, 2020

Marked as author ready. I do not think we have any CI that could test this, so we should not require a CI run.

Lite CI might be good just to make sure this doesn't break Jenkins in a surprising totally-unexpected way.

cclauss
cclauss requested changes Jan 7, 2020
View reviewed changes
Copy link
Contributor

@cclauss cclauss left a comment •
edited
Loading

Choose a reason for hiding this comment

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

After looking at exceptions in https://docs.python.org/3/library/contextlib.html, I no longer believe that the proposed change is equivalent to the current code. If shutil.copyfileobj() raises an exception, will inf.close() happen??

@Trott
Copy link
Member

Trott commented Jan 7, 2020 •
edited
Loading

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4119/

Lite CI: https://ci.nodejs.org/job/node-test-pull-request-lite-pipeline/4120/

Lite CI seems to be not working right now. Going to run a full CI.

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28266/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28279/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/28290/

Trott pushed a commit that referenced this pull request Jan 8, 2020
@Trott
build: avoid using CMP for BZ2File
bd6d651 
Some Python distributions do not support
context manager protocol (CMP) for BZ2File.

PR-URL: #31198
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
@Trott
Copy link
Member

Trott commented Jan 8, 2020

Landed in bd6d651

Thanks for the contribution! 🎉

@Trott Trott closed this Jan 8, 2020
MylesBorins pushed a commit that referenced this pull request Jan 16, 2020
@MylesBorins
build: avoid using CMP for BZ2File
9643718 
Some Python distributions do not support
context manager protocol (CMP) for BZ2File.

PR-URL: #31198
Reviewed-By: Anna Henningsen <[email protected]>
Reviewed-By: Ben Noordhuis <[email protected]>
Reviewed-By: James M Snell <[email protected]>
Reviewed-By: Rich Trott <[email protected]>
Reviewed-By: Christian Clauss <[email protected]>
@codebytere codebytere mentioned this pull request Jan 16, 2020
Merged
@codebytere
Copy link
Member

This looks like it needs a backport but it doesn't land cleanly at all - would someone from @nodejs/platform-windows be able to help since the original committer's account is gone now?

@targos targos removed author ready PRs that have at least one approval, no pending requests for changes, and a CI started. backport-requested-v12.x labels Apr 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees
No one assigned
Labels
build Issues and PRs related to build files or the CI.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Unable to build nodejs
9 participants
@BridgeAR @Trott @nodejs-github-bot @codebytere @bnoordhuis @jasnell @addaleax @cclauss @targos