Page MenuHomePhabricator

phabricator: use urllib3 to handle conduit HTTP
ClosedPublic

Authored by mjpieters on Oct 12 2017, 10:17 AM.

Details

Reviewers
ikostia
Group Reviewers
Restricted Project
Commits
rFBHGX9e01e91c5dd8: phabricator: use urllib3 to handle conduit HTTP
Summary

urlgrabber is not available on Windows machines; urllib3 is MIT licensed so can
safely be bundled.

Test Plan

Run the tests, run hg ssl against a repository with valid arcanist config.

Diff Detail

Repository
rFBHGX Facebook Mercurial Extensions
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mjpieters created this revision.Oct 12 2017, 10:17 AM
Herald added a reviewer: Restricted Project. · View Herald TranscriptOct 12 2017, 10:17 AM
mjpieters retitled this revision from Use urllib3 to handle conduit HTTP to phabricator: use urllib3 to handle conduit HTTP.Oct 12 2017, 10:26 AM
ikostia requested changes to this revision.Oct 12 2017, 10:27 AM

I would suggest separating these concerns:

  • making conduit API dependent on urllib3
  • adding urllib3 to the repo

The reasons are:

  • most platforms (including windows without a custom-built python) have better ways of resolving a urllib3 dependency
  • for FB, we usually put checked-in libs in the parent repo, facebook-hg-rpms
This revision now requires changes to proceed.Oct 12 2017, 10:27 AM
mjpieters updated this revision to Diff 2626.Oct 12 2017, 10:43 AM
ikostia accepted this revision.Oct 12 2017, 10:46 AM

Awesome, thanks!

This revision is now accepted and ready to land.Oct 12 2017, 10:46 AM
ikostia requested changes to this revision.Oct 17 2017, 10:26 AM
ikostia added inline comments.
phabricator/conduit.py
73–74

This comment need to be removed/changed as well.

This revision now requires changes to proceed.Oct 17 2017, 10:26 AM

Also, just applying this patch and building a Windows version does not work: the build is successful, but ssl output shows "Error" in place of a phabricator status.

mjpieters updated this revision to Diff 2931.Oct 17 2017, 12:49 PM

Also, just applying this patch and building a Windows version does not work: the build is successful, but ssl output shows "Error" in place of a phabricator status.

Can you share the traceback? You can disable the try...except urllib3.exceptions.HTTPError handling to see it. Perhaps we need to distribute certificates to Windows.

mjpieters marked an inline comment as done.Oct 17 2017, 12:52 PM

It is failing to decode JSON, since the response.data seems to be an HTML piece.

Here's the tail of the traceback:

  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\hgext3rd\phabstatus.py", line 140, in showphabstatus
    populateresponseforphab(repo, diffnum)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\hgext3rd\phabstatus.py", line 129, in populateresponseforphab
    getdiffstatus(repo, *okdiffnumbers)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\hgext3rd\phabstatus.py", line 50, in helper
    u = f(*args)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\hgext3rd\phabstatus.py", line 78, in getdiffstatus
    timeout=timeout)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\phabricator\conduit.py", line 183, in call_conduit
    return client.call(method, args, timeout=timeout)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\site-packages\phabricator\conduit.py", line 94, in call
    response = json.loads(response.data)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\json\__init__.py", line 339, in loads
    return _default_decoder.decode(s)
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\json\decoder.py", line 364, in decode
    obj, end = self.raw_decode(s, idx=_w(s, 0).end())
  File "C:\Code\fb-hg-rpms2\build\hg\hg-python\lib\json\decoder.py", line 382, in raw_decode
    raise ValueError("No JSON object could be decoded")
ValueError: No JSON object could be decoded
mjpieters updated this revision to Diff 2964.Oct 18 2017, 6:24 AM
mjpieters updated this revision to Diff 2965.Oct 18 2017, 6:31 AM
ikostia accepted this revision.Oct 18 2017, 6:40 AM
This revision is now accepted and ready to land.Oct 18 2017, 6:40 AM
This revision was automatically updated to reflect the committed changes.