Skip to content

/ webrender

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

update to bincode 1.0.0 #2394

Closed
wants to merge 1 commit into from
Closed

update to bincode 1.0.0 #2394

wants to merge 1 commit into from

Conversation

@TyOverby
Copy link
Contributor

TyOverby commented Feb 8, 2018 •
edited

This is a PR showing how to update to bincode 1.0.0

This change is Reviewable
@bors-servo
Copy link
Contributor

bors-servo commented Feb 9, 2018

☔ The latest upstream changes (presumably #2403) made this pull request unmergeable. Please resolve the merge conflicts.
@kvark
kvark approved these changes Feb 11, 2018
View changes
@Eijebong
Copy link
Member

Eijebong commented Feb 16, 2018

This should also update ipc-channel to 0.10 to avoid having two versions of bincode in servo
@TyOverby TyOverby changed the title DO NOT MERGE | update to bincode 1.0.0 update to bincode 1.0.0 Feb 26, 2018
@kvark
kvark reviewed Feb 27, 2018
View changes
Cargo.lock
@@ -391,6 +391,20 @@ dependencies = [
"servo-freetype-sys 4.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]
name = "fuchsia-zircon"

This comment has been minimized.

Sign in to view
@kvark

kvark Feb 27, 2018

Member

WR is going to ship parts of Fuchsia?

This comment has been minimized.

Sign in to view
@TyOverby

TyOverby Feb 27, 2018

Author Contributor

Looks like it's a new dependency of mio?

This comment has been minimized.

Sign in to view
@TyOverby

TyOverby Feb 27, 2018

Author Contributor

From Mio Cargo.toml

[target.'cfg(target_os = "fuchsia")'.dependencies]
fuchsia-zircon = "0.3.2"
fuchsia-zircon-sys = "0.3.2"

This comment has been minimized.

Sign in to view
@kvark

kvark Feb 27, 2018

Member

It has BSD-3-Clause license, which is atypical for Rust ecosystem. Gotta check with Gecko folks before merging, since all dependencies are vendored for Gecko, and licenses matter.

This comment has been minimized.

Sign in to view
@mbrubeck

mbrubeck Mar 7, 2018

Contributor

These fuchsia-zircon crates are already in the Gecko source tree because the latest version of rand depends on them too.
@TyOverby
Copy link
Contributor Author

TyOverby commented Feb 27, 2018

@kvark, @Eijebong Everything look good?
@Eijebong
Copy link
Member

Eijebong commented Feb 27, 2018

@TyOverby For me everything is looking good 👍 (can't r+) I'll do the necessary servo work once this lands.
@kvark
kvark reviewed Feb 27, 2018
View changes
Cargo.lock
@@ -391,6 +391,20 @@ dependencies = [
"servo-freetype-sys 4.0.3 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]
name = "fuchsia-zircon"

This comment has been minimized.

Sign in to view
@kvark

kvark Feb 27, 2018

Member

It has BSD-3-Clause license, which is atypical for Rust ecosystem. Gotta check with Gecko folks before merging, since all dependencies are vendored for Gecko, and licenses matter.
Cargo.lock
@@ -830,6 +846,16 @@ dependencies = [
"libc 0.2.20 (registry+https://github.com/rust-lang/crates.io-index)",
]

[[package]]

This comment has been minimized.

Sign in to view
@kvark

kvark Feb 27, 2018

Member

why do we need rand now?

This comment has been minimized.

Sign in to view
@Eijebong

Eijebong Feb 27, 2018

Member

rand is already in the dependency tree, this is just a dupe because some dependencies haven't updated yet (I've started doing what's needed for that)
@bors-servo
Copy link
Contributor

bors-servo commented Feb 27, 2018

☔ The latest upstream changes (presumably #2473) made this pull request unmergeable. Please resolve the merge conflicts.
@bors-servo
Copy link
Contributor

bors-servo commented Mar 6, 2018

☔ The latest upstream changes (presumably #2492) made this pull request unmergeable. Please resolve the merge conflicts.
@mbrubeck mbrubeck mentioned this pull request Mar 7, 2018
Closed
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

Anything I could do to get this moving again ?
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 13, 2018

can I regenerate the Cargo.lock file by just running cargo update or is that a bad idea?
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

Running a full cargo update is a pretty bad idea, you'll end up with a lot of unwanted stuff updated.
I usually reset the Cargo.lock to current master state and rerun my cargo update -p whatever command
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 13, 2018

How do I know which packages to cargo update -p?
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

Well you need to update ipc-channel and that's probably it (unless it won't update it in which case you'll have to add every ipc-channel dependency until it unlocks, it happens sometimes).
@staktrace
Copy link
Contributor

staktrace commented Mar 13, 2018

BTW BSD-3-Clause seems to be our list of acceptable licenses, https://searchfox.org/mozilla-central/rev/99df6dad057b94c2ac4d8b12f7b0b3a7fdf25d04/python/mozbuild/mozbuild/vendor_rust.py#147

So that's not a blocker, at least.
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 13, 2018 

Tys-MacBook-Pro-2:webrender tyoverby$ cargo update -p ipc-channel
    Updating registry `https://github.com/rust-lang/crates.io-index`
error: failed to select a version for `mio` (required by `ipc-channel`):
all possible versions conflict with previously selected versions of `mio`
  version 0.6.7 in use by mio v0.6.7
  possible versions to select: 0.6.14, 0.6.13, 0.6.12, 0.6.11
Tys-MacBook-Pro-2:webrender tyoverby$
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

Yeah, add mio to the updated crates list cargo update -p ipc-channel -p mio
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 13, 2018

@Eijebong I did that, and it got down to iovec or something, and then adding -p iovec just gave me the same iovec error again
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

@TyOverby Mhh going from master,

cargo update -p iovec -p mio -p ipc-channel -p bincode -p net2
cargo update -p mio --precise 0.6.12

I downgraded mio to avoid duping slab (I'll have a look at it later)
@Eijebong
Copy link
Member

Eijebong commented Mar 13, 2018

Also it would probably be better if those commits were all squashed :)
@glennw
Copy link
Member

glennw commented Mar 14, 2018

Is this ready for merge now? Do we need any sign-off to make sure that both Gecko and Servo are ready to get this merged?
@staktrace
Copy link
Contributor

staktrace commented Mar 14, 2018

Try push to make sure the gecko side is good:
https://treeherder.mozilla.org/#/jobs?repo=try&revision=d7f41b5ad16de72866a76a08bbedabe0f7fcbc33
@staktrace
Copy link
Contributor

staktrace commented Mar 14, 2018

Looks like there's some crashes but it's from the audioipc update which I'll be pushing along with the WR update since they both bump bincode. So I have no objections to merging this PR from the gecko side, although I'll need to get help on the audioipc crashes before we can actually push the WR update into gecko (or just do WR alone which will end up with two vendored bincode versions).
@Eijebong
Copy link
Member

Eijebong commented Mar 14, 2018

@staktrace https://github.com/djg/audioipc-2/pull/35/files#diff-f7e555878138f2fef856422bafa6439aR157 is the only thing that could crash in audioipc I think and I'm pretty sure that this unwrap is safe.
Do we have a backtrace or something like that ?
@staktrace
Copy link
Contributor

staktrace commented Mar 14, 2018

@Eijebong https://treeherder.mozilla.org/logviewer.html#?job_id=167943511&repo=try&lineNumber=9622 is the stacktrace. Note that the audioipc update includes other changes too, everything in the range 7e866e5ba304b271ab4c504a354ed7aa628343b8..b93386611d7d9689c4f0177a4704f0adc16bc2d1 is fair game.
@Eijebong
Copy link
Member

Eijebong commented Mar 14, 2018

Oh... This is way too domain specific for me to help :(
@staktrace
Copy link
Contributor

staktrace commented Mar 14, 2018

Oh, the crash is probably from djg/audioipc-2@ead320a which needs a gecko-side change. But that landed after the bincode update so I can just update audioipc to the bincode update and skip the audioipc_client_init change if needed.
@glennw
Copy link
Member

glennw commented Mar 14, 2018

Could we get this squashed into one commit (makes it easier to revert if we should ever need to), and then this is ready to merge.
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 14, 2018

@glennw: doesn't github have a "merge and squash" feature?
@Eijebong
Copy link
Member

Eijebong commented Mar 14, 2018

@TyOverby It does but they're using bors here to merge PRs instead of the github interface
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 14, 2018 •
edited

If I squash by using git rebase master -i, I have to go back and redo all my conflict resolutions. Is there a way to use git to avoid this?
@TyOverby
update bincode to 1.0
Loading status checks…
3efd45a
@TyOverby TyOverby force-pushed the TyOverby:bincode_1_0_update branch from 398406f to 3efd45a Mar 14, 2018
@TyOverby
Copy link
Contributor Author

TyOverby commented Mar 14, 2018

apparently git reset --soft master && git commit will do a squash without any weird resolution issues.
@glennw
Copy link
Member

glennw commented Mar 14, 2018

Thanks!

@bors-servo r+
@bors-servo
Copy link
Contributor

bors-servo commented Mar 14, 2018

📌 Commit 3efd45a has been approved by glennw
@bors-servo
Copy link
Contributor

bors-servo commented Mar 15, 2018

⌛ Testing commit 3efd45a with merge 6c4f78b...
bors-servo added a commit that referenced this pull request Mar 15, 2018
@bors-servo
Auto merge of #2394 - TyOverby:bincode_1_0_update, r=glennw
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
6c4f78b 
update to bincode 1.0.0

This is a PR showing how to update to bincode 1.0.0

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2394)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 15, 2018

💔 Test failed - status-travis
@glennw
Copy link
Member

glennw commented Mar 15, 2018

Blocked by #2522.
@jrmuizel
Copy link
Contributor

jrmuizel commented Mar 15, 2018

@bors-servo retry
bors-servo added a commit that referenced this pull request Mar 15, 2018
@bors-servo
Auto merge of #2394 - TyOverby:bincode_1_0_update, r=glennw
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
c0f639a 
update to bincode 1.0.0

This is a PR showing how to update to bincode 1.0.0

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2394)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

bors-servo commented Mar 15, 2018

⌛ Testing commit 3efd45a with merge c0f639a...
@bors-servo
Copy link
Contributor

bors-servo commented Mar 15, 2018

💔 Test failed - status-taskcluster
@Eijebong
Copy link
Member

Eijebong commented Mar 15, 2018

Need to add rand to the list of duped dependencies
@bors-servo
Copy link
Contributor

bors-servo commented Mar 16, 2018

☔ The latest upstream changes (presumably #2529) made this pull request unmergeable. Please resolve the merge conflicts.
@staktrace staktrace mentioned this pull request Mar 20, 2018
Merged
@staktrace
Copy link
Contributor

staktrace commented Mar 20, 2018

I rebased this patch, it's up on #2547 now.
bors-servo added a commit that referenced this pull request Mar 20, 2018
@bors-servo
Auto merge of #2547 - staktrace:bincode, r=glennw
Verified
This commit was created on GitHub.com and signed with a verified signature using GitHub’s key.
GPG key ID: 4AEE18F83AFDEB23 Learn about signing commits
Loading status checks…
941dc18 
Update to bincode 1.0.0

Rebase of #2394

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/webrender/2547)
<!-- Reviewable:end -->
@glennw
Copy link
Member

glennw commented Mar 20, 2018

Closing in favor of #2547.
@glennw glennw closed this Mar 20, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mbrubeck mbrubeck

@kvark kvark

@Eijebong Eijebong

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
@TyOverby @bors-servo @Eijebong @staktrace @glennw @jrmuizel @mbrubeck @kvark
You can’t perform that action at this time.