Solidify Python 3 support in the manifest code #23733
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
Oops, meant to mark this as a draft. Will certainly fail some tests |
jgraham
force-pushed
the
python3_type_branch
branch
from
79df9ed
to
f70b95f
Compare
jgraham
force-pushed
the
python3_type_branch
branch
from
f70b95f
to
2e99a78
Compare
jgraham
force-pushed
the
python3_type_branch
branch
from
2e99a78
to
363021b
Compare
wpt-pr-bot
added
the
wptrunner
jgraham
force-pushed
the
python3_type_branch
branch
from
363021b
to
c8130df
Compare
jgraham
force-pushed
the
python3_type_branch
branch
from
843d022
to
c091e4c
Compare
jgraham
force-pushed
the
python3_type_branch
branch
from
c091e4c
to
74a9e11
Compare
tools/manifest/download.py
Outdated
| @@ -159,7 +159,8 @@ def download_manifest( | |||
| fileobj = io.BytesIO(resp.read()) | |||
| try: | |||
| with gzip.GzipFile(fileobj=fileobj) as gzf: | |||
| decompressed = gzf.read() # type: ignore | |||
| data = read_gzf(gzf) | |||
There was a problem hiding this comment.
You need type: ignore here too.
The goal here is to get mypy passing in Python 3; prior to this patch there were a lot of cases where Python 3 behaviour was broken but not picked up by the tests (e.g. because it only happened when updating the manifest or with certain caches or similar). Many of these issues were seen by mypy, but that had been disabled for Py3 specfically to avoid having to fix all the issues at the time the annotations were added. But now we do need that support. The general approach here is to use the same types for Py 2 and Py 3. In particular we use Text where possible (with a couple of exceptions where it was easier not to), rather than bytes. The native str type is avoided wherever possible since this introduces difficult to debug differences between Python versions. The preference for Text includes paths, which means we no longer support running in non-unicode directories (in practice it is likely to be broken if the path isn't UTF-8 compatible on Unix or UTF-16 compatible on Windows).
Seems like bare pip started defaulting to python 3
jgraham
force-pushed
the
python3_type_branch
branch
from
5c7a3dd
to
5fb7ee9
Compare
stephenmcgruer
approved these changes
Jul 6, 2020
| RUN pip3 install --upgrade pip | ||
| RUN pip3 install virtualenv | ||
|
|
||
| # Ensure we have up-to-date six |
There was a problem hiding this comment.
Just noting that this still needs addressed as far as I can tell (as in, filing a followup issue or sending a followup PR)
Hexcles
approved these changes
Jul 6, 2020
There was a problem hiding this comment.
FTR, the overall change LGTM. My only concerns back then were the mypy breakage which seems to have been addressed. I'm assuming Stephen's proposed fixes were adopted (not sure since the branch was rebased). These fixes LGTM, too. IMHO, they are not "horrific hacks", but thoughtful workarounds :) Thanks again for the in-depth investigation here and getting this unblocked, Stephen!
gsnedders
added a commit
to gsnedders/web-platform-tests
that referenced
this pull request
Sep 30, 2021
At least at times, we could end up with the rename limit warning being output (i.e., `inexact rename detection was skipped due to too many files`). This then caused our assert to fail, as it relied on the output following the -z argument and ending without any trailing out. This warning is absolutely useless for us, as we're just looking to find out what files have changed, so we don't care if the file is a rename or not, so we can avoid it by just outright disabling rename detection. This was previously fixed in web-platform-tests#18879 (30c42ec) by just outright ignoring stderr, but was regressed in web-platform-tests#23733 (6539f1e). Avoiding the warning being output in the first place seems like a better solution than ignoring any errors git might produce.
jgraham
pushed a commit
that referenced
this pull request
Oct 1, 2021
At least at times, we could end up with the rename limit warning being output (i.e., `inexact rename detection was skipped due to too many files`). This then caused our assert to fail, as it relied on the output following the -z argument and ending without any trailing out. This warning is absolutely useless for us, as we're just looking to find out what files have changed, so we don't care if the file is a rename or not, so we can avoid it by just outright disabling rename detection. This was previously fixed in #18879 (30c42ec) by just outright ignoring stderr, but was regressed in #23733 (6539f1e). Avoiding the warning being output in the first place seems like a better solution than ignoring any errors git might produce.
moz-v2v-gh
pushed a commit
to mozilla/gecko-dev
that referenced
this pull request
Oct 5, 2021
… on TC, a=testonly Automatic update from web-platform-tests Disable rename detection when looking for changed tests At least at times, we could end up with the rename limit warning being output (i.e., `inexact rename detection was skipped due to too many files`). This then caused our assert to fail, as it relied on the output following the -z argument and ending without any trailing out. This warning is absolutely useless for us, as we're just looking to find out what files have changed, so we don't care if the file is a rename or not, so we can avoid it by just outright disabling rename detection. This was previously fixed in web-platform-tests/wpt#18879 (30c42ecf9a) by just outright ignoring stderr, but was regressed in web-platform-tests/wpt#23733 (6539f1e343). Avoiding the warning being output in the first place seems like a better solution than ignoring any errors git might produce. -- Deal with the different semantics of A..B in git-diff in repo_files_changed At the moment on TaskCluster we pass something like {PR base}..{PR head} which is sometimes a small range when parsed as a range (see gitrevisions(7)), as it is everything reachable from PR head but not from PR base. By comparison, for git-diff, A..B gives all changes between the two trees A and B, which if the PR is filed with a merge-base a long way behind master would be a much larger change. As such, change repo_files_changed to use the merge-base of A..B, as this gives us the diff of the commits that are new on the PR branch. -- wpt-commits: 7cba72c881e666578f1f01c790d2298dd3ee756d, a7514a7da22289bccf9e0ce5e7bc28f75b9dc9bc wpt-pr: 31037
jamienicol
pushed a commit
to jamienicol/gecko
that referenced
this pull request
Oct 6, 2021
… on TC, a=testonly Automatic update from web-platform-tests Disable rename detection when looking for changed tests At least at times, we could end up with the rename limit warning being output (i.e., `inexact rename detection was skipped due to too many files`). This then caused our assert to fail, as it relied on the output following the -z argument and ending without any trailing out. This warning is absolutely useless for us, as we're just looking to find out what files have changed, so we don't care if the file is a rename or not, so we can avoid it by just outright disabling rename detection. This was previously fixed in web-platform-tests/wpt#18879 (30c42ecf9a) by just outright ignoring stderr, but was regressed in web-platform-tests/wpt#23733 (6539f1e343). Avoiding the warning being output in the first place seems like a better solution than ignoring any errors git might produce. -- Deal with the different semantics of A..B in git-diff in repo_files_changed At the moment on TaskCluster we pass something like {PR base}..{PR head} which is sometimes a small range when parsed as a range (see gitrevisions(7)), as it is everything reachable from PR head but not from PR base. By comparison, for git-diff, A..B gives all changes between the two trees A and B, which if the PR is filed with a merge-base a long way behind master would be a much larger change. As such, change repo_files_changed to use the merge-base of A..B, as this gives us the diff of the commits that are new on the PR branch. -- wpt-commits: 7cba72c881e666578f1f01c790d2298dd3ee756d, a7514a7da22289bccf9e0ce5e7bc28f75b9dc9bc wpt-pr: 31037
Gabisampaio
pushed a commit
to Gabisampaio/wpt
that referenced
this pull request
Nov 18, 2021
At least at times, we could end up with the rename limit warning being output (i.e., `inexact rename detection was skipped due to too many files`). This then caused our assert to fail, as it relied on the output following the -z argument and ending without any trailing out. This warning is absolutely useless for us, as we're just looking to find out what files have changed, so we don't care if the file is a rename or not, so we can avoid it by just outright disabling rename detection. This was previously fixed in web-platform-tests#18879 (30c42ec) by just outright ignoring stderr, but was regressed in web-platform-tests#23733 (6539f1e). Avoiding the warning being output in the first place seems like a better solution than ignoring any errors git might produce.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
No description provided.