Closed Bug 1369156 Opened 7 years ago Closed 7 years ago

Remove ./third_party/rust/bindgen/Cargo.toml.orig

Categories

(Developer Services :: Servo VCS Sync, defect)

Product:
Developer Services
Component:
Servo VCS Sync
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: MatsPalmgren_bugz, Assigned: kats)

References

Details

Attachments

(3 files, 1 obsolete file)

Result of running mach vendor rust with cargo-vendor 0.1.10
127.63 KB, text/plain
Details
Bug 1369156 - Bump the minimum allowed version of cargo-vendor.
59 bytes, text/x-review-board-request
froydnj
: review+
Details
Bug 1369156 - Re-vendor third-party rust libraries with latest cargo-vendor.
59 bytes, text/x-review-board-request
froydnj
: review+
Details 
This seems like a mistake:

# hg log ./third_party/rust/bindgen/Cargo.toml.orig
changeset:   361399:23515b00893b
user:        Servo VCS Sync <servo-vcs-sync@mozilla.com>
date:        Tue May 30 19:18:04 2017 +0000
summary:     No bug - Revendor rust dependencies

It's a bad idea in general to have files named *.orig and *.rej in the tree
since those extensions are used by mercurial for conflicts.  Especially in
this case when there is a third_party/rust/bindgen/Cargo.toml file, which
is why I assume the above is a mistake.
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/c1a21b5a5175
Remove accidentally committed Cargo.toml.orig; r=me
Blocks: 1369161
And this got reverted a few seconds later by Servo VCS Sync: https://hg.mozilla.org/integration/autoland/rev/254e04d927d5

So obviously the vcs sync server is in a "bad" state and it is failing to clean up from that. This bug is worse in scope than I thought.
Component: Build Config → Servo VCS Sync
Product: Core → Developer Services
The Cargo.toml.orig file is actually in the packaged crate. STR:

wget https://crates.io/api/v1/crates/bindgen/0.25.3/download
mv download bindgen.tar.gz
tar tf bindgen.tar.gz | grep "Cargo.toml"

The same thing happened with plane-split, I filed kvark/plane-split#3 for that. In general the `cargo publish` step will publish whatever random crap you happen to have in your directory to crates.io.
(The correct fix here is to have the bindgen maintainer republish to crates.io a version that doesn't include the Cargo.toml.orig, and then revendor that new version into m-c).
Hah. I should have checked for the obvious bug before assuming this was vcs sync being buggy. Fool me once, shame on me. Fool me twice, confefe.
https://hg.mozilla.org/mozilla-central/rev/c1a21b5a5175
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Still an issue.
Status: RESOLVED → REOPENED
Resolution: FIXED → ---
Also for the record there are actually 4 crates with this problem:

kats@kgupta-air mozilla$ find . -name "*.orig"
./third_party/rust/bindgen/Cargo.toml.orig
./third_party/rust/rayon/Cargo.toml.orig
./third_party/rust/rayon-core/Cargo.toml.orig
./third_party/rust/url/Cargo.toml.orig
There's more now as well. Can we get some eyes on this? It is annoying when you want to clean up .orig files from a repo.
I hit this recently. I have a script that removes all the .rej and .orig files in my tree, because they tend to pile up, so my patch ended up with changes I didn't expect.
Attached patch 1369156_1.patch (obsolete) — — Splinter Review 
could we just ignore .orig and .rej files?

even if they get accidentally included in a crate, we won't then commit them to our repo.


no idea who to ask for a review :)
Comment on attachment 8879453 [details] [diff] [review]
1369156_1.patch

Review of attachment 8879453 [details] [diff] [review]:
-----------------------------------------------------------------

I don't think this will work as intended. When we vendor third-party crates into the tree the vendoring tool also adds a .cargo-checksum.json file which contains a hash of all the files in the crate. That json will still include the hash for the .orig files even with this patch, but the .orig files won't get added to the tree. That will result in a mismatch and bustage.
Attachment #8879453 - Flags: review-
The correct patch, if you want to modify the vendoring tool, would be to skip .orig files at [1].

[1] https://github.com/alexcrichton/cargo-vendor/blob/master/src/main.rs#L288
Alex, do you have suggestions on what the best fix here is?
- Change cargo to not generate .orig files
- Change cargo to not package the .orig files into the published crate
- Have each project manually remove their .orig files and re-publish
- Change cargo-vendor to ignore .orig files (or just Cargo.toml.orig files)
- Other
Flags: needinfo?(acrichton)
Hm AFAIK neither Cargo nor cargo-vendor creates `*.orig` files. If these files are coming from crates.io then this is a bug with upstream authors, otherwise I'm not sure where the orig files are coming from :(
Flags: needinfo?(acrichton)
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #14)
> The correct patch, if you want to modify the vendoring tool, would be to
> skip .orig files at [1].
> 
> [1] https://github.com/alexcrichton/cargo-vendor/blob/master/src/main.rs#L288

i'll give this a shot.
Attachment #8879453 - Attachment is obsolete: true
(In reply to Alex Crichton [:acrichto] from comment #16)
> Hm AFAIK neither Cargo nor cargo-vendor creates `*.orig` files. If these
> files are coming from crates.io then this is a bug with upstream authors,
> otherwise I'm not sure where the orig files are coming from :(

You added the code! https://github.com/rust-lang/cargo/pull/4030#issuecomment-301127300 :)
Flags: needinfo?(acrichton)
Wow sorry about that! Apparently I have a very short memory.

I'll merge https://github.com/alexcrichton/cargo-vendor/pull/44 and publish pronto
Flags: needinfo?(acrichton)
Thanks! Let me know when it's published and I can write a patch to bump the min cargo-vendor version and re-vendor all the things.
Oh sure yeah, 0.1.10 should now be on crates.io and tagged, should be good to go!
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #21)
> Thanks! Let me know when it's published and I can write a patch to bump the
> min cargo-vendor version and re-vendor all the things.

So I did this locally. Turns out the next step in this yak-shaving adventure is to figure out why cargo-vendor is suddenly deleting a whole pile of crates that we still need.
(In reply to Kartikaya Gupta (email:kats@mozilla.com) from comment #23)
> So I did this locally. Turns out the next step in this yak-shaving adventure
> is to figure out why cargo-vendor is suddenly deleting a whole pile of
> crates that we still need.

This is (unsurprisingly) a result of https://github.com/alexcrichton/cargo-vendor/commit/4e7e56a38fb6bb810816122e577b377d1f186f05. We'll need to add --no-delete to our invocation of cargo vendor.
I also filed/fixed alexcrichton/cargo-vendor#45 for another issue I found. With that fix in, we now need cargo-vendor v0.1.11 and things are looking good.
Assignee: nobody → bugmail
Rebased the patches onto autoland tip, because otherwise they have merge conflicts with bug 1373381 which I just landed there.
Comment on attachment 8879654 [details]
Bug 1369156 - Bump the minimum allowed version of cargo-vendor.

https://reviewboard.mozilla.org/r/151016/#review155858
Attachment #8879654 - Flags: review?(nfroyd) → review+
Comment on attachment 8879655 [details]
Bug 1369156 - Re-vendor third-party rust libraries with latest cargo-vendor.

https://reviewboard.mozilla.org/r/151018/#review155860

Thanks for helping to make the vendoring situation better!
Attachment #8879655 - Flags: review?(nfroyd) → review+
We're sorry, Autoland could not rebase your commits for you automatically. Please manually rebase your commits and try again.

hg error in cmd: hg rebase -s a2f2f48a2dbd -d 75b728151a2c: rebasing 402944:a2f2f48a2dbd "Bug 1369156 - Bump the minimum allowed version of cargo-vendor. r=froydnj"
rebasing 402945:d68046146d94 "Bug 1369156 - Re-vendor third-party rust libraries with latest cargo-vendor. r=froydnj" (tip)
merging third_party/rust/cookie/.cargo-checksum.json
merging third_party/rust/webdriver/.cargo-checksum.json
warning: conflicts while merging third_party/rust/cookie/.cargo-checksum.json! (edit, then use 'hg resolve --mark')
warning: conflicts while merging third_party/rust/webdriver/.cargo-checksum.json! (edit, then use 'hg resolve --mark')
unresolved conflicts (see hg resolve, then hg rebase --continue)
Pushed by kgupta@mozilla.com:
https://hg.mozilla.org/integration/autoland/rev/a012b5a75d3c
Bump the minimum allowed version of cargo-vendor. r=froydnj
https://hg.mozilla.org/integration/autoland/rev/f7af9bce9793
Re-vendor third-party rust libraries with latest cargo-vendor. r=froydnj
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: