◐ Shell
reader mode source ↗
Skip to content

gh-95023: Added os.setns and os.unshare to easily switch between namespaces on Linux#95046

Merged
vstinner merged 48 commits into
python:mainfrom
noamcohen97:namespaces
Oct 20, 2022
Merged

gh-95023: Added os.setns and os.unshare to easily switch between namespaces on Linux#95046
vstinner merged 48 commits into
python:mainfrom
noamcohen97:namespaces

Conversation

@noamcohen97

@noamcohen97 noamcohen97 commented Jul 20, 2022 •
edited by bedevere-bot
Loading

Copy link
Copy Markdown
Contributor

@bedevere-bot

bedevere-bot commented Jul 20, 2022

Copy link
Copy Markdown

Most changes to Python require a NEWS entry.

Please add it using the blurb_it web app or the blurb command-line tool.

@noamcohen97 noamcohen97 force-pushed the namespaces branch 2 times, most recently from 868fe76 to 92a6d73 Compare July 20, 2022 13:21
@noamcohen97 noamcohen97 marked this pull request as ready for review July 20, 2022 13:26
@serhiy-storchaka serhiy-storchaka self-requested a review July 21, 2022 07:25
@MaxwellDupre

MaxwellDupre commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

I would like to understand more about why I would want to use these functions, how to use and caveats. Just providing the bear functions with out examples and descriptions seems like half a job. Due to the current excellent documentation and current direction of the docs.

@erlend-aasland

erlend-aasland commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

@MaxwellDupre:

Just providing the bear functions with out examples and descriptions seems like half a job.

There is no need for such a negative tone; please try to be more encouraging when leaving review comments. Noam has followed the dev flow by creating an issue, explaining the need for this change, and has provided a very well written patch, complete with docs, tests, and a NEWS entry. I think he has done a great job, and I welcome his interest in making CPython better.

I welcome your interest in helping review PRs. Reviewing PRs is a difficult task; often more difficult than writing the code (or the docs) itself. I know the devguide is lacking when it comes to "how to review PRs". For now, the best thing to do is IMO to observe how others, more experienced reviewers, perform this task, and learn from them. Personally, I often find reviewing PRs difficult, and I often use far more time when reviewing a PR than I would use to write one.

@MaxwellDupre

MaxwellDupre commented Jul 21, 2022

Copy link
Copy Markdown
Contributor

Understand, I did think about how to phrase and just went with the shortest. I realise how it maybe seen as negative and will try to improve.

@bedevere-bot

bedevere-bot commented Jul 26, 2022

Copy link
Copy Markdown

A Python core developer has requested some changes be made to your pull request before we can consider merging it. If you could please address their requests along with any other requests in other reviews from core developers that would be appreciated.

Once you have made the requested changes, please leave a comment on this pull request containing the phrase I have made the requested changes; please review again. I will then notify any core developers who have left a review that you're ready for them to take another look at this pull request.

And if you don't make the requested changes, you will be poked with soft cushions!

@noamcohen97

noamcohen97 commented Jul 26, 2022

Copy link
Copy Markdown
Contributor Author

I have made the requested changes; please review again.

75 hidden items Load more…
@CAM-Gerlach

CAM-Gerlach commented Aug 23, 2022

Copy link
Copy Markdown
Member

@tiran @erlend-aasland ping

@erlend-aasland

erlend-aasland commented Aug 23, 2022

Copy link
Copy Markdown
Contributor

I'm waiting for Christian, since he requested changes. For me, the PR is good to go.

@noamcohen97

noamcohen97 commented Sep 9, 2022

Copy link
Copy Markdown
Contributor Author

@tiran ?

@noamcohen97

noamcohen97 commented Oct 8, 2022

Copy link
Copy Markdown
Contributor Author

#98056 fixed the failing tests

@CAM-Gerlach

CAM-Gerlach commented Oct 12, 2022

Copy link
Copy Markdown
Member

I've pinged @tiran again out of band

@encukou encukou added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 12, 2022
@bedevere-bot

bedevere-bot commented Oct 12, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @encukou for commit b14396e 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

@bedevere-bot bedevere-bot removed the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 12, 2022
@encukou

encukou commented Oct 12, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

As far as I know, Christian is quite busy these days. Don't wait for him too long if he didn't request that you do. He can always do a post-merge review.

I started a buildbot check, since this looks like it might have platform-specific issues.

@noamcohen97

noamcohen97 commented Oct 14, 2022

Copy link
Copy Markdown
Contributor Author

@encukou Looks like failures are related to #98216
I have updated the branch, can you started the buildbot check again please?

@CAM-Gerlach

CAM-Gerlach commented Oct 15, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

I've triggered a re-run of the buildbots

@CAM-Gerlach CAM-Gerlach added the 🔨 test-with-buildbots Test PR w/ buildbots; report in status section label Oct 15, 2022
@bedevere-bot

bedevere-bot commented Oct 15, 2022

Copy link
Copy Markdown

🤖 New build scheduled with the buildbot fleet by @CAM-Gerlach for commit 50c4809 🤖

If you want to schedule another build, you need to add the ":hammer: test-with-buildbots" label again.

vstinner
vstinner reviewed Oct 18, 2022
View reviewed changes

@vstinner vstinner left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Hide comment

The doc might be elaborated, but since there are young functions, I'm fine with redirecting users to their manual pages. Here is another review on the test.

Overall, the change LGTM. But I would prefer a few more adjustements of the test before approving it.

@vstinner vstinner merged commit a371a7e into python:main Oct 20, 2022
@vstinner

vstinner commented Oct 20, 2022

Copy link
Copy Markdown
Member

Ok, enough nitpicking. This version is good enough to be merged. We can always enhance tests and doc later if needed. Thanks a lot @noamcohen97 for updating your PR many times. I like it and merged your PR.

unshare() and setns() are important functions nowadays on Linux. By the way, I like the "unshare" command line tool ;-)

carljm added a commit to carljm/cpython that referenced this pull request Oct 20, 2022
@carljm
Merge branch 'main' into typewatch …
8bba5da 
* main: (40 commits)
  pythongh-98461: Fix source location in comprehensions bytecode (pythonGH-98464)
  pythongh-98421: Clean Up PyObject_Print (pythonGH-98422)
  pythongh-98360: multiprocessing now spawns children on Windows with correct argv[0] in virtual environments (pythonGH-98462)
  CODEOWNERS: Become a typing code owner (python#98480)
  [doc] Improve logging cookbook example. (pythonGH-98481)
  Add more tkinter.Canvas tests (pythonGH-98475)
  pythongh-95023: Added os.setns and os.unshare functions (python#95046)
  pythonGH-98363: Presize the list for batched() (pythonGH-98419)
  pythongh-98374: Suppress ImportError for invalid query for help() command. (pythongh-98450)
  typing tests: `_overload_dummy` raises `NotImplementedError`, not `RuntimeError` (python#98351)
  pythongh-98354: Add unicode check for 'name' attribute in _imp_create_builtin (pythonGH-98412)
  pythongh-98257: Make _PyEval_SetTrace() reentrant (python#98258)
  pythongh-98414: py.exe launcher does not use defaults for -V:company/ option (pythonGH-98460)
  pythongh-98417: Store int_max_str_digits on the Interpreter State (pythonGH-98418)
  Doc: Remove title text from internal links (python#98409)
  [doc] Refresh the venv introduction documentation, and correct the statement about VIRTUAL_ENV (pythonGH-98350)
  Docs: Bump sphinx-lint and fix unbalanced inline literal markup (python#98441)
  pythongh-92886: Replace assertion statements in `handlers.BaseHandler` to support running with optimizations (`-O`) (pythonGH-93231)
  pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `_test_multiprocessing.py` (pythonGH-93233)
  pythongh-92886: Fix tests that fail when running with optimizations (`-O`) in `test_py_compile.py` (pythonGH-93235)
  ...
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@vstinner vstinner vstinner left review comments

@erlend-aasland erlend-aasland erlend-aasland approved these changes

@CAM-Gerlach CAM-Gerlach CAM-Gerlach approved these changes

@serhiy-storchaka serhiy-storchaka Awaiting requested review from serhiy-storchaka

@tiran tiran Awaiting requested review from tiran

+2 more reviewers

@mwichmann mwichmann mwichmann left review comments

@bskinn bskinn bskinn left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

10 participants

@noamcohen97 @bedevere-bot @MaxwellDupre @erlend-aasland @bskinn @tiran @CAM-Gerlach @encukou @vstinner @mwichmann