Make pkg defs Python 2/3 compatible #3850
Conversation
|
Can one of the admins verify this patch? |
|
This will be able to be tested properly once #3873 is merged. |
hlopko
added
category: skylark > pkg build defs
P3
|
Jekins: test this please. Thanks! I'll merge it if tests result are ok. |
|
Can one of the admins verify this patch? |
|
Looks like a timeout in an (afaict) unrelated test. Is that one known to be flaky? |
|
Sorry there was some reimaging going on, let's retry to get an answer from our CI: retest this please |
|
So the first of those checks is the same timeout as before. The second, most of the workspaces it's trying to build with are listed as missing. The third didn't fail when I submitted the request, but appears to be because the contents of //third_party/pprof aren't included in //third_party:srcs, which must have been introduced in 8e04f24 |
|
there is a problem: you can ignore everyone but bazel-tests but bazel-tests get cancelled all the time, let me retry (I deactivated the other ones): test this please |
|
Appears to have again failed on the same tests as master. I don't think any of the CI is going to be able to test this fully until #3873 goes through, though, as the current config only makes sure it's compatible from the Python 2 side. Iif this is merged now, can I request that #3873 is rebuilt so I know I've not introduced any fun new failures? |
|
Ok the failure are now genuine but coming from the baseline.
LGTM (will import tomorrow morning).
…On Wed, Oct 11, 2017, 5:49 PM treuherz ***@***.***> wrote:
Appears to have again failed on the same tests as master. I don't think
any of the CI is going to be able to test this fully until #3873
<#3873> goes through, though, as
the current config only makes sure it's compatible from the Python 2 side.
Iif this is merged now, can I request that #3873
<#3873> is rebuilt so I know I've
not introduced any fun new failures?
—
You are receiving this because you were assigned.
Reply to this email directly, view it on GitHub
<#3850 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/ADjHfx45AmdL6n_09CLlmC1t2aYNNukZks5srOOggaJpZM4PqtfD>
.
|
|
And this is breaking internally due to six imports... I need to take a closer look at it. Might take a bit longer. |
|
Anything I can do to help here? |
|
This requires weird change to third-party, I need to investigate more. Sorry for the delay |
|
Sorry to keep nagging. Has there been any movement on this? |
|
Sorry I am leaving the Bazel team. This change require some non negligeable effort from the Bazel team to import. Reassigning to @philwo to opine |
Use BytesIO instead of StringIO, change strings to bytes throughout the archiving code. Needed to import from Six in a couple of places.
|
Have rebased against current master, and removed use of six, since that appeared to be a problem. Some of the updates to master since I did this have incorporated the same changes, so the diff's a bit smaller now, but master is still importing StringIO in py3-incompatible ways, and using it where it should be using BytesIO. @philwo Can I request a retest? |
|
Jekins: test this please |
tools/build_defs/pkg/make_deb.py
Outdated
| '\x60\x0a', # end of file entry | ||
| ] | ||
| for i in inputs: | ||
| fileobj.write(i.encode()) |
There was a problem hiding this comment.
I think we should explicitly specify the encoding as ascii or utf-8, instead of leaving it ambiguous. If we choose utf-8, then we have to encode first, then justify to 8 or 16 bytes. It looks like this is only called with ascii filenames, though.
|
General comment for Bazel team - there is no unit test for make_deb.py, so breakages like this aren't caught, even by the Python-3-specific CI runs. |
UTF-8 for tar, ASCII for ar, as appears to be the standard
|
That should be fixed. Don't think ar would know a two-byte character if it hit it in the face, so have stuck with Ascii for that one to be safe. |
|
Jenkins: test this please. |
|
Can one of the admins verify this patch? |
|
Appears to have failed on something unrelated again, "test.xml.[failed-to-read]". |
|
Jenkins: test this please. |
1 similar comment
|
Jenkins: test this please. |
|
Sorry for the delay, I was out sick for some time. |
|
No worries, thanks for sorting this! |
Use BytesIO instead of StringIO, change strings to bytes throughout the
archiving code. Needed to import from Six in a couple of places.
As discussed in #1580