Using `arc diff` on Windows results in empty binary files landing

RESOLVED FIXED

Status

defect
P1
normal
RESOLVED FIXED
10 months ago
5 months ago

People

(Reporter: zombie, Assigned: imadueme)

Tracking

({conduit-triaged, conduit-upstream})

Firefox Tracking Flags

(Not tracked)

Details

()

Attachments

(1 attachment)

Reporter

Description

10 months ago
Posted file arc.log
This is from bug 1369209 and https://phabricator.services.mozilla.com/D3106 from which Lando produced https://hg.mozilla.org/integration/autoland/rev/29cdfd4dec23

It got backed out twice because of the failing tests, and I had to land manually.

I think I saw "empty file" in Phabricator yesterday, but I can't find where I saw it today, so I can't confirm if it's a problem with arc or Lando.

Attached is the result of `arc diff --trace`, though I don't see anything obvious.
Attachment #9003833 - Attachment mime type: text/x-log → text/plain
I ran into something very similar on https://phabricator.services.mozilla.com/D5566, where I was trying to update an existing binary file and the diff that ended up in Phabricator was empty. Fortunately my reviewer caught the problem before I landed anything. I don't have a lot of other information to add, but at least this rules out Lando as being the problem, since I didn't get that far.
Reporter

Comment 2

9 months ago
It happened again https://phabricator.services.mozilla.com/D6999

Is anyone looking into this?  The current solution is really inconvenient.
Reporter

Comment 3

9 months ago
Abandoned in 2015 https://secure.phabricator.com/D10994

Sigh.
Reporter

Comment 4

9 months ago
Sorry, wrong link, abandoned in january 2018:

https://secure.phabricator.com/D15675#231103
Flags: needinfo?(mcote)
I've filed a ticket upstream.  We have a support contract with them so hopefully we can get it fixed before too long.  I'll post another update here when I hear back.
Flags: needinfo?(mcote)
Keywords: conduit-triaged
Whiteboard: [phabricator-upstream]
Looks like work is now happening in https://secure.phabricator.com/T13209.
Is there any update here? Seems like work on the linked issue stalled three weeks ago.
Flags: needinfo?(mcote)
Would it be reasonable to make a quick patch to just loudly fail when trying to deal with binary files on Windows, if this is going to take a while to fix?
(In reply to Dave Townsend [:mossop] (he/him) from comment #7)
> Is there any update here? Seems like work on the linked issue stalled three
> weeks ago.

I've pinged upstream.  I think the delay is due to the fact that these fixes are part of their very large Arcanist redesign/rewrite/rejiggering.

(In reply to Adam Gashlin [:agashlin] from comment #8)
> Would it be reasonable to make a quick patch to just loudly fail when trying
> to deal with binary files on Windows, if this is going to take a while to
> fix?

I'm not sure if there's a way to do a "quick patch" here, but I'll look into it.
Flags: needinfo?(mcote)
A fix for this landed and was released last week: https://secure.phabricator.com/rARC83661809e532c3fe444a8bf7c7d6936e6377691b

We'll deploy it soon.
This should be fixed now.  Note that you'll need to upgrade arcanist first (the fix has also been merged into our git-cinnabar fork).
Status: NEW → RESOLVED
Closed: 7 months ago
Resolution: --- → FIXED
Looks like this may not actually be fixed.  See bug 1501991.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

Updated

7 months ago
Assignee: nobody → imadueme
Priority: -- → P1
Assignee

Comment 13

7 months ago
Hi Johann - the uploading of empty binary diffs to Phabricator should have been fixed. One thing to note is that the fix is at the client side (arcanist, which moz-phab uses under the hood), not on the Phabricator server (nor is it a bug with Lando).

Can you confirm that you have the latest version of arcanist? Run `arc --version`, it should be later than:

  arcanist 3534d2baca4bf6dcdac46c49164bf5ba3a6660ad (8 Nov 2018)
  libphutil 335a9f8c2dce649e24acf903dd4dec2bfbc1e9aa (8 Nov 2018)

If you do not have the latest version of arcanist you can upgrade by running `arc upgrade`, or if that fails by recloning the arcanist repo wherever you initially installed it.

After upgrading arcanist, it is necessary to re-submit those diffs which before had an empty binary file. When I inspected the raw diff of https://phabricator.services.mozilla.com/D11611 (click the 'Download Raw Diff' button in the right sidebar), I still see empty binary files which is what led me to believe that you had not upgraded your arcanist client.

Please let us know if you think this is still an issue! If it is can you post your arcanist version, OS, and whether you use arcanist/moz-phab/phlay/phabsend?
Flags: needinfo?(jhofmann)
(In reply to Israel Madueme [:imadueme] from comment #13)
> Hi Johann - the uploading of empty binary diffs to Phabricator should have
> been fixed. One thing to note is that the fix is at the client side
> (arcanist, which moz-phab uses under the hood), not on the Phabricator
> server (nor is it a bug with Lando).
> 
> Can you confirm that you have the latest version of arcanist? Run `arc
> --version`, it should be later than:
> 
>   arcanist 3534d2baca4bf6dcdac46c49164bf5ba3a6660ad (8 Nov 2018)
>   libphutil 335a9f8c2dce649e24acf903dd4dec2bfbc1e9aa (8 Nov 2018)
> 
> If you do not have the latest version of arcanist you can upgrade by running
> `arc upgrade`, or if that fails by recloning the arcanist repo wherever you
> initially installed it.
> 
> After upgrading arcanist, it is necessary to re-submit those diffs which
> before had an empty binary file. When I inspected the raw diff of
> https://phabricator.services.mozilla.com/D11611 (click the 'Download Raw
> Diff' button in the right sidebar), I still see empty binary files which is
> what led me to believe that you had not upgraded your arcanist client.
> 
> Please let us know if you think this is still an issue! If it is can you
> post your arcanist version, OS, and whether you use
> arcanist/moz-phab/phlay/phabsend?

Ah, yes, I use phabsend and have not updated it (not sure if a fix is available for that). Sorry, I wasn't aware that this was fixed on the client.
Flags: needinfo?(jhofmann)
(In reply to Israel Madueme [:imadueme] from comment #13)
> Can you confirm that you have the latest version of arcanist? Run `arc
> --version`, it should be later than:
> 
>   arcanist 3534d2baca4bf6dcdac46c49164bf5ba3a6660ad (8 Nov 2018)
>   libphutil 335a9f8c2dce649e24acf903dd4dec2bfbc1e9aa (8 Nov 2018)

Should arcanist be later than 3534d2? That seems to be the latest at https://github.com/phacility/arcanist
They release from their stable branch.  We upgrade our installation every one or two weeks.  The hashes Israel mentioned are from what is currently on our systems (second-latest release, week 44; we're upgrading to week 45 this week).
Assignee

Comment 17

7 months ago
Arcanist should be at or later than the 83661809e532c3fe444a8bf7c7d6936e6377691b (Oct 26) commit, I had an even newer commit locally, sorry for the confusion.

Closing this bug since it seems like it is a problem with phabsend, which we don't officially support. You can use arcanist or mozphab to submit changes that have binary files. Please let us know if it happens again and we'll continue investigating!
Status: REOPENED → RESOLVED
Closed: 7 months ago7 months ago
Resolution: --- → FIXED
Whiteboard: [phabricator-upstream]
You need to log in before you can comment on or make changes to this bug.