Skip to content

gh-107888: Fix test_mmap.test_access_parameter() on macOS 14 #109928

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

Merged
merged 1 commit into from
Sep 26, 2023

Conversation

vstinner
Copy link
Member

@vstinner vstinner commented Sep 26, 2023

@vstinner
Copy link
Member Author

!buildbot ARM64 macOS

@bedevere-bot
Copy link

🤖 New build scheduled with the buildbot fleet by @vstinner for commit 7b401f4 🤖

The command will test the builders whose names match following regular expression: ARM64 macOS

The builders matched are:

  • ARM64 macOS PR

@vstinner
Copy link
Member Author

cc @ned-deily

Copy link
Member

@ned-deily ned-deily left a comment

Choose a reason for hiding this comment

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

Thanks, LGTM, with a blurb should be good to go. This probably should be backported to security releases as well to avoid CI/buildbot failures.

try:
m = mmap.mmap(f.fileno(), mapsize, prot=prot)
except PermissionError:
# on macOS 14, PROT_READ | PROT_WRITE is not allowed
Copy link
Member

Choose a reason for hiding this comment

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

perhaps make this a self.skipTest message?

Copy link
Member

Choose a reason for hiding this comment

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

perhaps make this a self.skipTest message?

That's probably a good idea although there are plenty of other places in test_mmap that simply do a pass for similar types of failures.

Copy link
Member Author

Choose a reason for hiding this comment

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

test_access_parameter() is a long function: 124 lines, it contains many tests. If someone wants to use skipTest() as some places, the function should be splitted into sub-functions. Otherwise, there is a risk that newly added code below will be skipped by mistake.

Today I lost 10 min trying to understand why the flush() call that I added at the end doesn't work as expected...

def display_header(use_resources: tuple[str, ...]):
    ...  # <=== a lot more code obviously

    # This makes it easier to remember what to set in your local
    # environment when trying to reproduce a sanitizer failure.
    asan = support.check_sanitizer(address=True)
    msan = support.check_sanitizer(memory=True)
    ubsan = support.check_sanitizer(ub=True)
    sanitizers = []
    if asan:
        sanitizers.append("address")
    if msan:
        sanitizers.append("memory")
    if ubsan:
        sanitizers.append("undefined behavior")
    if not sanitizers:
        return

    print(f"== sanitizers: {', '.join(sanitizers)}")
    for sanitizer, env_var in (
        (asan, "ASAN_OPTIONS"),
        (msan, "MSAN_OPTIONS"),
        (ubsan, "UBSAN_OPTIONS"),
    ):
        options= os.environ.get(env_var)
        if sanitizer and options is not None:
            print(f"== {env_var}={options!r}")

    sys.stdout.flush()  # <====== ADDED CODE ======

@vstinner
Copy link
Member Author

buildbot/ARM64 macOS PR

test_gh105829_should_not_deadlock_if_wakeup_pipe_full() of test_concurrent_futures.test_deadlock decided to hang: issue gh-109917. This failure is unrelated to test_mmap (unrelated to this change).

@vstinner vstinner merged commit 9dbfe2d into python:main Sep 26, 2023
@vstinner vstinner deleted the test_mmap branch September 26, 2023 22:26
@miss-islington
Copy link
Contributor

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.11, 3.12.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 26, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 26, 2023

GH-109929 is a backport of this pull request to the 3.12 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.12 only security fixes label Sep 26, 2023
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Sep 26, 2023
@bedevere-app
Copy link

bedevere-app bot commented Sep 26, 2023

GH-109930 is a backport of this pull request to the 3.11 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.11 only security fixes label Sep 26, 2023
@vstinner
Copy link
Member Author

test_gh105829_should_not_deadlock_if_wakeup_pipe_full() of test_concurrent_futures.test_deadlock decided to hang: issue #109917. This failure is unrelated to test_mmap (unrelated to this change).

I'm not curious if this bug only occurs on macOS or not. I'm asking since I couldn't reproduce it on Linux.

@vstinner
Copy link
Member Author

Thanks for quick reviews @ned-deily and @gpshead!

@ned-deily
Copy link
Member

As I hinted at in the review, this might be worthy of a blurb as others will probably run into it. What do you think?

@vstinner
Copy link
Member Author

As I hinted at in the review, this might be worthy of a blurb as others will probably run into it. What do you think?

Oh. These days I'm fixing so many test failures that I stopped documenting most of them. I'm busy fixing more test failures. If you want, please go ahead and document the fix.

vstinner added a commit that referenced this pull request Sep 26, 2023
…H-109928) (#109930)

gh-107888: Fix test_mmap.test_access_parameter() on macOS 14 (GH-109928)
(cherry picked from commit 9dbfe2d)

Co-authored-by: Victor Stinner <[email protected]>
vstinner added a commit to miss-islington/cpython that referenced this pull request Sep 29, 2023
Yhg1s pushed a commit that referenced this pull request Oct 2, 2023
…H-109928) (#109929)

gh-107888: Fix test_mmap.test_access_parameter() on macOS 14 (GH-109928)
(cherry picked from commit 9dbfe2d)

Co-authored-by: Victor Stinner <[email protected]>
@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.8.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.9.
🐍🍒⛏🤖

@miss-islington-app
Copy link

Thanks @vstinner for the PR 🌮🎉.. I'm working now to backport this PR to: 3.10.
🐍🍒⛏🤖

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 17, 2024
miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 17, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jan 17, 2024

GH-114183 is a backport of this pull request to the 3.8 branch.

miss-islington pushed a commit to miss-islington/cpython that referenced this pull request Jan 17, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jan 17, 2024

GH-114184 is a backport of this pull request to the 3.9 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.9 only security fixes label Jan 17, 2024
@bedevere-app
Copy link

bedevere-app bot commented Jan 17, 2024

GH-114185 is a backport of this pull request to the 3.10 branch.

@bedevere-app bedevere-app bot removed the needs backport to 3.10 only security fixes label Jan 17, 2024
ambv pushed a commit that referenced this pull request Jan 17, 2024
ambv pushed a commit that referenced this pull request Jan 17, 2024
ambv pushed a commit that referenced this pull request Jan 17, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
skip news tests Tests in the Lib/test dir
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants