Skip to content
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

Kill serde_codegen 💣 #15769

Merged
merged 1 commit into from Feb 28, 2017
Merged

Kill serde_codegen 💣 #15769

merged 1 commit into from Feb 28, 2017

Conversation

@nox
Copy link
Member

nox commented Feb 28, 2017

This change is Reviewable

@jdm
Copy link
Member

jdm commented Feb 28, 2017

@highfive
Copy link

highfive commented Feb 28, 2017

Heads up! This PR modifies the following files:

  • @asajeffrey: components/constellation/Cargo.toml
  • @KiChjang: components/net/Cargo.toml, components/script/Cargo.toml, components/net_traits/Cargo.toml, components/net_traits/Cargo.toml
  • @fitzgen: components/script/Cargo.toml
  • @emilio: components/layout/Cargo.toml
@highfive
Copy link

highfive commented Feb 28, 2017

warning Warning warning

  • These commits modify net, layout, gfx, and script code, but no tests are modified. Please consider adding a test!
@highfive highfive assigned jdm and unassigned asajeffrey Feb 28, 2017
@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2017

📌 Commit f8467a2 has been approved by jdm

@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2017

⌛ Testing commit f8467a2 with merge 74e7afa...

bors-servo added a commit that referenced this pull request Feb 28, 2017
Kill serde_codegen 💣

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

bors-servo commented Feb 28, 2017

💔 Test failed - arm32

@nox
Copy link
Member Author

nox commented Feb 28, 2017

Network issues, waiting for the other builds to finish.

@glennw
Copy link
Member

glennw commented Feb 28, 2017

@bors-servo retry

@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2017

⚡ Previous build results for android, arm64, linux-dev, linux-rel-css, mac-dev-unit, mac-rel-css, mac-rel-wpt1, windows-gnu-dev, windows-msvc-dev are reusable. Rebuilding only arm32, linux-rel-wpt, mac-rel-wpt2...

@bors-servo
Copy link
Contributor

bors-servo commented Feb 28, 2017

☀ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-gnu-dev, windows-msvc-dev
Approved by: jdm
Pushing 74e7afa to master...

@bors-servo bors-servo merged commit f8467a2 into servo:master Feb 28, 2017
2 of 3 checks passed
2 of 3 checks passed
continuous-integration/travis-ci/pr The Travis CI build failed
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
homu Test successful
Details
@edmorley
Copy link

edmorley commented Mar 1, 2017

As a head's up, the commit message here contained a non-BMP unicode character ("astral unicode character"), which requires 4 bytes to represent rather than the 3 bytes of BMP unicode, and as such causes issues with MySQL tables using utf8. This is affecting Treeherder pushlog ingestion (https://bugzilla.mozilla.org/show_bug.cgi?id=1115608) - we'll either need to strip out the non-BMP unicode or else switch this particular table to utf8mb4.

Obviously Treeherder should definitely handle this case gracefully, but there are may be other systems that consume the hg.mozilla.org pushlog that may not handle it well either, so please bear it in mind when using more unusual unicode characters in commit messages :-)

@nox nox deleted the nox:die-codegen-die branch Mar 1, 2017
@nox
Copy link
Member Author

nox commented Mar 1, 2017

@edmorley How was it not a problem in the past though? This is not the first time I use emojis.

@edmorley
Copy link

edmorley commented Mar 1, 2017

I imagine they were within the BMP (Basic Multilingual Plane) unicode range:
https://en.wikipedia.org/wiki/Plane_(Unicode)

(or else only in the PR description and not the commit message)

@nox
Copy link
Member Author

nox commented Mar 1, 2017

I'm pretty sure U+1F916 ROBOT FACE is outside the BMP.

@nox
Copy link
Member Author

nox commented Mar 1, 2017

Oh right, you squash all commits of a PR together, right?

@SimonSapin
Copy link
Member

SimonSapin commented Mar 1, 2017

we'll either need to [
] switch this particular table to utf8mb4.

Yes. It’s been in MySQL since 2010.

I personally don’t think that “Only use BMP characters” is much more acceptable than “Only use ASCII characters”.

As to PR descriptions, the get included by bors/homu in the message of merge commits.

@edmorley
Copy link

edmorley commented Mar 1, 2017

I agree, it's just unfortunate that:
(a) MySQL makes it way too easy to fall into this trap by defaulting to latin1 thus making people have to pick a charset themselves - which is easy to get wrong due to: their unfortunate naming of BMP-only utf8 as utf8, the sheer volume of docs online that don't even mention utf8mb4, and their not deprecating the BMP-only charsets. And after all that they then make people who want to convert utf8->utf8mb4 jump through hoops to support longer indexes (eg the innodb_file_format=barracuda, innodb_large_prefix=1 dance).
(b) Django doesn't support utf8mb4 out of the box (https://code.djangoproject.com/ticket/18392; not helped by MySQL being seen as a 2nd-class citizen by the Django project compared to PostgreSQL)

As another datapoint, bugzilla.mozilla.org is currently facing similar "utf8 doesn't mean utf8 in MySQL" realisations / hassle converting:
https://bugzilla.mozilla.org/show_bug.cgi?id=1253535
https://bugzilla.mozilla.org/show_bug.cgi?id=1328659

That said, I think the answer is just to give up on MySQL (this is just the last in a long list of annoyances we've had with it) and use PostgreSQL. But not an overnight switch for large existing projects like Treeherder. One day! :-)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants
You can’t perform that action at this time.