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

Run on Rust stable #151

Merged
merged 8 commits into from
Jul 31, 2015
Merged

Run on Rust stable #151

merged 8 commits into from
Jul 31, 2015

Conversation

SimonSapin
Copy link
Member

When the "unstable" Cargo feature is given, html5ever’s build script will use html5ever_macros as a library to generate or update src/tree_builder/rules.expanded.rs, which is part of the source tree.

(This feature also enables the corresponding features of tendril and string-cache.)

html5ever_macros is not a plugin anymore, but it still uses libsyntax and still behaves a lot like a syntax extension.

When "unstable" is not given, the build script checks that rules.expanded.rs is up to date (based on a hash of rules.rs) and fails if it’s not.

Tests also run on stable Rust on Travis-CI, but unfortunately most of them are disabled as they dynamically generated using internals of the test crate.

r? @Manishearth

Fixes #53
Closes #151

Review on Reviewable

@SimonSapin
Copy link
Member Author

Don’t be deterred by the +2,936 lines diff stat, 2,497 of those are src/tree_builder/rules.expanded.rs :)

@Ygg01
Copy link
Contributor

Ygg01 commented Jul 27, 2015

@SimonSapin: I'm wondering, is #112 still active? Will html5ever move to aster in future?

@SimonSapin
Copy link
Member Author

I made this close #112, as #112 (comment) indicates that the motivation was stable Rust.

@Manishearth
Copy link
Member

Reviewed 8 of 8 files at r1.
Review status: 4 of 39 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 7 of 7 files at r2.
Review status: 6 of 39 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 1 of 1 files at r3, 5 of 6 files at r4.
Review status: 7 of 39 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 1 of 6 files at r4.
Review status: 7 of 39 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 1 of 1 files at r5, 9 of 9 files at r6, 18 of 18 files at r7.
Review status: 24 of 39 files reviewed at latest revision, all discussions resolved, all commit checks successful.


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 6 of 6 files at r8, 2 of 2 files at r9, 2 of 2 files at r10, 6 of 6 files at r11, 1 of 1 files at r12, 3 of 3 files at r13, 1 of 1 files at r14.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


examples/noop-tokenize.rs, line 29 [r11] (raw file):
Do we have benchmarks? Because this would affect them. It's also possible to write your own blackbox via FFI.


examples/print-tree-actions.rs, line 126 [r11] (raw file):
Can we leave a FIXME here?


macros/src/pre_expand.rs, line 48 [r14] (raw file):
no need for as_str here


tests/tokenizer.rs, line 425 [r9] (raw file):
Why did we do this earlier?


Comments from the review on Reviewable.io

@SimonSapin
Copy link
Member Author

Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.


examples/noop-tokenize.rs, line 29 [r11] (raw file):
There is benches/tokenizer.rs, but it doesn’t use this file. (It also looks like it’s not built in CI.)


examples/print-tree-actions.rs, line 126 [r11] (raw file):
Done


macros/src/pre_expand.rs, line 48 [r14] (raw file):
Removed.


tests/tokenizer.rs, line 426 [r9] (raw file):
main is not significant when building with --test, the real main function is provided by the test harness. Previously, #[start] was used to override that main function. I removed usage of #[start] because it is unstable, but that’s kinda moot since I didn’t manage to completely port this file to stable. (It uses the test crate to generate tests dynamically.)

So, with 70e3039 we have one test harness instance that finds a single test that happens to be called main and creates another test harness instance. The output looks like:

     Running target/debug/tokenizer-dcf7263c41100ab8

running 1 test

running 13344 tests
test tok: contentModelFlags.test: End tag closing RCDATA or RAWTEXT (case-insensitivity) (in state RawData(Rawtext)) ... ok
test tok: contentModelFlags.test: End tag closing RCDATA or RAWTEXT (case-insensitivity) (in state RawData(Rcdata)) ... ok
[many more lines…]
test tb: webkit02.dat-7 ... ok

test result: ok. 1338 passed; 0 failed; 211 ignored; 0 measured

test main ... ok

test result: ok. 1 passed; 0 failed; 0 ignored; 0 measured

Maybe we’re better off reverting to using #[start] until we find a way to actually run these tests on stable Rust?


Comments from the review on Reviewable.io

@Manishearth
Copy link
Member

Reviewed 2 of 2 files at r15, 1 of 1 files at r16.
Review status: all files reviewed at latest revision, 1 unresolved discussion, some commit checks pending.


Comments from the review on Reviewable.io

When the "unstable" Cargo feature is given, html5ever’s build script
will use html5ever_macros as a library to generate or update
src/tree_builder/rules.expanded.rs, which is part of the source tree.

(This feature also enables the corresponding features of tendril and
string-cache.)

html5ever_macros is not a plugin anymore, but it still uses libsyntax
and still behaves a lot like a syntax extension.

When "unstable" is *not* given, the build script checks that
rules.expanded.rs is up to date (based on a hash of rules.rs) and fails
if it’s not.
owned_dom was an interesting experiment, but there is no use for it.
... even though tokenizer and tree builder test are disabled
as they are created dynamically using internals of the `test` crate.
@SimonSapin
Copy link
Member Author

@bors-servo r=Manishearth

@bors-servo
Copy link
Contributor

📌 Commit d8c698a has been approved by Manishearth

@bors-servo
Copy link
Contributor

⌛ Testing commit d8c698a with merge 7df9461...

bors-servo pushed a commit that referenced this pull request Jul 31, 2015
Run on Rust stable

When the "unstable" Cargo feature is given, html5ever’s build script will use html5ever_macros as a library to generate or update `src/tree_builder/rules.expanded.rs`, which is part of the source tree.

(This feature also enables the corresponding features of tendril and string-cache.)

html5ever_macros is not a plugin anymore, but it still uses libsyntax and still behaves a lot like a syntax extension.

When "unstable" is *not* given, the build script checks that rules.expanded.rs is up to date (based on a hash of rules.rs) and fails if it’s not.

Tests also run on stable Rust on Travis-CI, but unfortunately most of them are disabled as they dynamically generated using internals of the `test` crate.

r? @Manishearth

Fixes #53
Closes #151

<!-- Reviewable:start -->
[<img src="https://reviewable.io/review_button.png" height=40 alt="Review on Reviewable"/>](https://reviewable.io/reviews/servo/html5ever/151)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

☀️ Test successful - travis

@bors-servo bors-servo merged commit d8c698a into master Jul 31, 2015
@SimonSapin SimonSapin deleted the stable branch July 31, 2015 14:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants