-
Notifications
You must be signed in to change notification settings - Fork 4.1k
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
[Bazel] Fix mobile-install for python2 #12540
Conversation
All (the pull request submitter and all commit authors) CLAs are signed, but one or more commits were authored or co-authored by someone other than the pull request submitter. We need to confirm that all authors are ok with their commits being contributed to this project. Please have them confirm that by leaving a comment that contains only Note to project maintainer: There may be cases where the author cannot leave a comment, or the comment is not properly detected as consent. In those cases, you can manually confirm consent of the commit author(s), and set the ℹ️ Googlers: Go here for more info. |
a895069
to
56df4ba
Compare
@@ -32,7 +32,7 @@ | |||
|
|||
import hashlib | |||
import os | |||
from queue import Queue | |||
from Queue import Queue |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should we use
try:
import Queue from Queue
except ImportError:
import queue as Queue
here to support both python 2 and 3?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
try:
from Queue import Queue
except ImportError:
import queue as Queue
This is the syntax that worked when python 2 is used.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@ThomasCJY can you change the code to the alternative suggested by @arunkumar9t2 ? That'll make this work in both Pyton2 and 3.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
updated @arunkumar9t2 @odracirnumira ptal
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The change looks reasonable to me. Unfortunately I lack the necessary permissions to stamp the diff. I think @ahumesky is the right person to loop in here?
Any updates on this? pretty trivial change... |
**Background** It turns out the queue module name is inconsistent across different python versions. We found that: * On macos system - python2 contains both queue and Queue module. - python3 only contains queue module * On Linux system - python2 contains only Queue module. - python3 only contains queue module Therefore, some developers are seeing `ImportError: No module named queue` errors locally on linux machine. **Change** Import correct Queue module instead **Test** Local test pass Upstream PR: bazelbuild#12540 --- Automatic squash commit from https://github.sc-corp.net/Snapchat/bazel/pull/83 Cooled by jchen
11d30e7
to
ec62e96
Compare
I've imported this change internally, but our linters are pretty unhappy about imports "not at top of file". We seem to be doing this in other places so I think I can work around it. |
looks like this is causing the //tools/android:build_incremental_dexmanifest_test test to fail:
(and also causing 2 internal tests to fail with the same error) Looks like this is because the original code Are these the 3 scenarios?
At least on my Linux machine, scenario 3 doesn't seem to work. There appears to be both
So unless something is different on macos, seems this change should be:
|
@ahumesky yeah you are right. It's a bit confusing but according to the python doc: python 2 uses python 3 uses so the import syntax should be
|
@ahumesky is this good to go? |
**Background** Fix issue introduced by bazelbuild@1049fe8 It turns out the queue module name is inconsistent across different python versions. We found that: * On some macos system: - python2 contains both queue and Queue module. All python2 contains Queue module - python3 only contains queue module * On some Linux system - python2 contains only Queue module. - python3 only contains queue module Therefore, some developers are seeing `ImportError: No module named queue` errors locally on linux machine after using mobile-install. **Change** Import correct Queue module instead **Test** Local test pass Closes bazelbuild#12540. PiperOrigin-RevId: 369773133
**Background** Fix issue introduced by 1049fe8 It turns out the queue module name is inconsistent across different python versions. We found that: * On some macos system: - python2 contains both queue and Queue module. All python2 contains Queue module - python3 only contains queue module * On some Linux system - python2 contains only Queue module. - python3 only contains queue module Therefore, some developers are seeing `ImportError: No module named queue` errors locally on linux machine after using mobile-install. **Change** Import correct Queue module instead **Test** Local test pass Closes #12540. PiperOrigin-RevId: 369773133
**Background** Fix issue introduced by bazelbuild@1049fe8 It turns out the queue module name is inconsistent across different python versions. We found that: * On some macos system: - python2 contains both queue and Queue module. All python2 contains Queue module - python3 only contains queue module * On some Linux system - python2 contains only Queue module. - python3 only contains queue module Therefore, some developers are seeing `ImportError: No module named queue` errors locally on linux machine after using mobile-install. **Change** Import correct Queue module instead **Test** Local test pass Closes bazelbuild#12540. PiperOrigin-RevId: 369773133
**Background** Fix issue introduced by bazelbuild@1049fe8 It turns out the queue module name is inconsistent across different python versions. We found that: * On some macos system: - python2 contains both queue and Queue module. All python2 contains Queue module - python3 only contains queue module * On some Linux system - python2 contains only Queue module. - python3 only contains queue module Therefore, some developers are seeing `ImportError: No module named queue` errors locally on linux machine after using mobile-install. **Change** Import correct Queue module instead **Test** Local test pass Closes bazelbuild#12540. PiperOrigin-RevId: 369773133
Background
Fix issue introduced by 1049fe8
It turns out the queue module name is inconsistent across different python versions. We found that:
Therefore, some developers are seeing
ImportError: No module named queue
errors locally on linux machine after using mobile-install.Change
Import correct Queue module instead
Test
Local test pass