Movatterモバイル変換


[0]ホーム

URL:


Skip to content

Navigation Menu

Sign in
Appearance settings

Search code, repositories, users, issues, pull requests...

Provide feedback

We read every piece of feedback, and take your input very seriously.

Saved searches

Use saved searches to filter your results more quickly

Sign up
Appearance settings

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

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 ourterms of service andprivacy statement. We’ll occasionally send you account related emails.

Already on GitHub?Sign in to your account

Merged
vstinner merged 48 commits intopython:mainfromnoamcohen97:namespaces
Oct 20, 2022

Conversation

noamcohen97
Copy link
Contributor

@noamcohen97noamcohen97 commentedJul 20, 2022
edited by bedevere-bot
Loading

@bedevere-bot
Copy link

Most changes to Pythonrequire a NEWS entry.

Please add it using theblurb_it web app or theblurb command-line tool.

@noamcohen97noamcohen97force-pushed thenamespaces branch 2 times, most recently from868fe76 to92a6d73CompareJuly 20, 2022 13:21
@noamcohen97noamcohen97 marked this pull request as ready for reviewJuly 20, 2022 13:26
@serhiy-storchakaserhiy-storchaka self-requested a reviewJuly 21, 2022 07:25
@MaxwellDupre
Copy link
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
Copy link
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 moreencouraging 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.

serhiy-storchaka, JelleZijlstra, and dorukcan reacted with thumbs up emojinoamcohen97 reacted with laugh emoji

@MaxwellDupre
Copy link
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.

erlend-aasland reacted with thumbs up emoji

@bedevere-bot
Copy link

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 phraseI 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
Copy link
ContributorAuthor

I have made the requested changes; please review again.

@CAM-Gerlach
Copy link
Member

@tiran@erlend-aasland ping

@erlend-aasland
Copy link
Contributor

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

CAM-Gerlach and noamcohen97 reacted with thumbs up emoji

@noamcohen97
Copy link
ContributorAuthor

@tiran ?

@noamcohen97
Copy link
ContributorAuthor

#98056 fixed the failing tests

@CAM-Gerlach
Copy link
Member

I've pinged@tiran again out of band

noamcohen97 reacted with thumbs up emoji

@encukouencukou added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 12, 2022
@bedevere-bot
Copy link

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

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 12, 2022
@encukou
Copy link
Member

encukou commentedOct 12, 2022
edited
Loading

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 and erlend-aasland reacted with thumbs up emoji

@noamcohen97
Copy link
ContributorAuthor

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

guybensimhon reacted with thumbs down emoji

@CAM-Gerlach
Copy link
Member

CAM-Gerlach commentedOct 15, 2022
edited
Loading

I've triggered a re-run of the buildbots

noamcohen97 and erlend-aasland reacted with thumbs up emoji

@CAM-GerlachCAM-Gerlach added the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 15, 2022
@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by@CAM-Gerlach for commit50c4809 🤖

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

@bedevere-botbedevere-bot removed the 🔨 test-with-buildbotsTest PR w/ buildbots; report in status section labelOct 15, 2022
Copy link
Member

@vstinnervstinner left a comment

Choose a reason for hiding this comment

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

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.

noamcohen97 reacted with thumbs up emoji

if rc == 2:
e = int(err[0])
self.assertIn(e, (errno.EPERM, errno.EINVAL, errno.ENOSPC, errno.ENOSYS))
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to move these checks inside the child process code. I don't think that skipping the test if unshare() or setns() fails matters. Just exit the child process early with a success (exit code 0, well, just exit) if you get one of these errors, no? If the child process must always succeed, you can use test.suppot.script_helper.assert_python_ok() which captures stdout and stderr and raises an exception with all data if the process fails.

Maybe it would make sense to add aexcept PermissionError: instead. Would you mind to add a comment explaining why/how PermissionError can happen?

ENOSPC is expected on unshare() if we exceed some limits on namespaces? If it can only happen on unshare(), maybe add a try/except only on this function call?

Why is EINVAL expected? Can it be raised if there is a bug in the test? Or can it be raised depending on the kernel configuration?

noamcohen97 reacted with laugh emoji
Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

Cool! did not know aboutassert_python_ok

Copy link
ContributorAuthor

Choose a reason for hiding this comment

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

EINVAL could be raised the kernel was not configured with theCONFIG_UTS_NS option.

@vstinnervstinner merged commita371a7e intopython:mainOct 20, 2022
@vstinner
Copy link
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 ;-)

noamcohen97 and erlend-aasland reacted with hooray emojinoamcohen97 reacted with heart emoji

carljm added a commit to carljm/cpython that referenced this pull requestOct 20, 2022
* 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 freeto join this conversation on GitHub. Already have an account?Sign in to comment
Reviewers

@vstinnervstinnervstinner left review comments

@mwichmannmwichmannmwichmann left review comments

@bskinnbskinnbskinn left review comments

@erlend-aaslanderlend-aaslanderlend-aasland approved these changes

@CAM-GerlachCAM-GerlachCAM-Gerlach approved these changes

@serhiy-storchakaserhiy-storchakaAwaiting requested review from serhiy-storchaka

@tirantiranAwaiting requested review from tiran

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

[8]ページ先頭

©2009-2025 Movatter.jp