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

Status: RESOLVED FIXED

(Conduit :: Phabricator, defect, P1)

Description

[Tomislav Jovanovic :zombie]

Attachment: 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.

Comment 1

[Molly Howell (she/her) (no longer active)]

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.

Comment 2

[Tomislav Jovanovic :zombie]

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

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

Comment 3

[Tomislav Jovanovic :zombie]

Abandoned in 2015 https://secure.phabricator.com/D10994

Sigh.

Comment 4

[Tomislav Jovanovic :zombie]

Sorry, wrong link, abandoned in january 2018:

https://secure.phabricator.com/D15675#231103

Comment 5

[Mark Côté [: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.

Comment 6

[Mark Côté [:mcote]]

Looks like work is now happening in https://secure.phabricator.com/T13209.

Comment 7

[Dave Townsend [:mossop]]

Is there any update here? Seems like work on the linked issue stalled three weeks ago.

Comment 8

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

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?

Comment 9

[Mark Côté [:mcote]]

(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.

Comment 10

[Mark Côté [:mcote]]

A fix for this landed and was released last week: https://secure.phabricator.com/rARC83661809e532c3fe444a8bf7c7d6936e6377691b

We'll deploy it soon.

Comment 11

[Mark Côté [:mcote]]

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).

Comment 12

[Mark Côté [:mcote]]

Looks like this may not actually be fixed.  See bug 1501991.

Comment 13

[Israel Madueme [:imadueme]]

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?

Comment 14

[Johann Hofmann [:johannh]]

(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.

Comment 15

[Adam Gashlin (he/him) [:agashlin] (ex-moco)]

(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

Comment 16

[Mark Côté [:mcote]]

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).

Comment 17

[Israel Madueme [:imadueme]]

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!