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

Make the style crate (almost) build with a stable compiler #11816

Merged
merged 4 commits into from Jun 22, 2016

Conversation

@SimonSapin
Copy link
Member

commented Jun 21, 2016

The bulk of this is adding cargo features to make derived implementations of heapsize and serde traits optional.

"Almost" because std::intrinsics::discriminant_value is currently unstable and doesn’t have any stable replacement that I know of. For now, this PR conditionally replaces it with unimplemented!().

r? @nox


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix are part of #11815 (github issue number if applicable).
  • There are tests for these changes OR
  • These changes do not require tests have tests yet because that requires #11806, but I still want to land this before it bitrots. (Tests should check that the build succeeds with a stable compiler.)

This change is Reviewable

@Ms2ger

This comment has been minimized.

Copy link
Contributor

commented Jun 21, 2016

Should be trivial to replace std::intrinsics::discriminant_value with a hand-written method.

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2016

You mean one per enum type rather than a generic one, right? And we’d probably want to auto-generate them with Mako or something.

@SimonSapin SimonSapin force-pushed the stable-style branch from 1690f13 to 467ff5c Jun 21, 2016
@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Jun 21, 2016

As a side-effect, this removes a bunch of crates from the geckolib build. (Dependencies of util that are now made optional and only enabled for servo.)

@SimonSapin SimonSapin force-pushed the stable-style branch from 47831be to 6cb9e45 Jun 21, 2016
@KiChjang

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Did highfive not assign a reviewer?

@wafflespeanut

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

Highfive intermittent: servo/highfive#124

@Ms2ger

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

Oh, I only recalled the call in viewport.rs.

@nox

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

r=me but I would like us to remove the unimplemented!() before it lands, and if we could keep the features to just "servo" I think it would be better.

-S-awaiting-review +S-awaiting-answer


Reviewed 20 of 20 files at r1, 40 of 40 files at r2, 2 of 2 files at r3, 8 of 8 files at r4.
Review status: all files reviewed at latest revision, all discussions resolved.


Comments from Reviewable

@nox nox self-assigned this Jun 22, 2016
@nox

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

📌 Commit db9e424 has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

⌛️ Testing commit db9e424 with merge c648255...

bors-servo added a commit that referenced this pull request Jun 22, 2016
Make the style crate (almost) build with a stable compiler

<!-- Please describe your changes on the following line: -->

The bulk of this is adding cargo features to make derived implementations of `heapsize` and `serde` traits optional.

"Almost" because `std::intrinsics::discriminant_value` is currently unstable and doesn’t have any stable replacement that I know of. For now, this PR conditionally replaces it with `unimplemented!()`.

r? @nox

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes <s>fix</s> are part of #11815 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not <s>require tests</s> *have tests yet* because that requires #11806, but I still want to land this before it bitrots. (Tests should check that the build succeeds with a stable compiler.)

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

💔 Test failed - linux-dev

@SimonSapin

This comment has been minimized.

Copy link
Member Author

commented Jun 22, 2016

@bors-servo r=nox

Updated lock file.

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

📌 Commit c48ce46 has been approved by nox

@nox

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

@bors-servo r-

Put the lockfile updates in the commit they belong.

Testing this on CI to make sure we don’t regress it is blocked on #11806
SimonSapin added 3 commits Jun 21, 2016
`discriminant_value` will need to be replaced with something else later.
... instead of fine-grained heap_size and serde-serialization ones.
@SimonSapin SimonSapin force-pushed the stable-style branch from ff6a670 to b103e8b Jun 22, 2016
@nox

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

📌 Commit b103e8b has been approved by nox

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

⌛️ Testing commit b103e8b with merge 0fb5d63...

bors-servo added a commit that referenced this pull request Jun 22, 2016
Make the style crate (almost) build with a stable compiler

<!-- Please describe your changes on the following line: -->

The bulk of this is adding cargo features to make derived implementations of `heapsize` and `serde` traits optional.

"Almost" because `std::intrinsics::discriminant_value` is currently unstable and doesn’t have any stable replacement that I know of. For now, this PR conditionally replaces it with `unimplemented!()`.

r? @nox

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [x] `./mach build -d` does not report any errors
- [x] `./mach test-tidy` does not report any errors
- [x] These changes <s>fix</s> are part of #11815 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not <s>require tests</s> *have tests yet* because that requires #11806, but I still want to land this before it bitrots. (Tests should check that the build succeeds with a stable compiler.)

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

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

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

💔 Test failed - linux-rel

@KiChjang

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

The first failure is #11703.

@jdm

This comment has been minimized.

Copy link
Member

commented Jun 22, 2016

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

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

@bors-servo

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2016

@bors-servo bors-servo merged commit b103e8b into master Jun 22, 2016
2 checks passed
2 checks passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
@bors-servo bors-servo referenced this pull request Jun 22, 2016
3 of 3 tasks complete
@SimonSapin SimonSapin deleted the stable-style branch Jun 22, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
8 participants
You can’t perform that action at this time.