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.

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

Bug 1770791 - Remove duplicate initialization in python glean package #2064

Merged
merged 2 commits into from
May 24, 2022

Conversation

arai-a
Copy link
Contributor

@arai-a arai-a commented May 23, 2022

for bug 1770791

On Windows, Creating FdLogger twice causes the subsequent subprocess operation somehow fail.
And apparently the duplicate initialization is completely unnecessary.
So, applied 2 fixes, both on Rust side and Python side.

Rust side's glean_enable_logging_to_fd uses OnceCell to prevent subsequent call overwriting the value,
but fd_logger::FdLogger::new(fd) is not guarded.
So, it should check FD_LOGGER.get().is_some() before fd_logger::FdLogger::new(fd).

Python side calls setup_logging twice, first time from _ffi.py top-level, and second time from glean.py top-level.
Either of them should be removed, and I've removed the former one.

The attached testcase fails on windows, with py.test -s tests/test_subprocess.py.
the -s option seems to be necessary to hit the problematic case.

@auto-assign auto-assign bot requested a review from badboy May 23, 2022 19:51
@arai-a arai-a force-pushed the py-subprocess branch 3 times, most recently from 180f7de to 89d12de Compare May 23, 2022 20:25
glean-core/src/lib.rs Outdated Show resolved Hide resolved
@badboy
Copy link
Member

badboy commented May 24, 2022

Turns out our Python Windows tests have been broken for a while. Grmpf.
(See https://app.circleci.com/pipelines/github/mozilla/glean/9332/workflows/86672023-5c22-450d-b756-7904a2b3a1a5/jobs/173298 -- they crash, but don't report that as an error value).

Gonna go with your fix though, and fix CI in a followup.

@badboy badboy enabled auto-merge (rebase) May 24, 2022 09:05
@badboy badboy disabled auto-merge May 24, 2022 09:37
@badboy badboy merged commit edfa0a4 into mozilla:main May 24, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants