Test themes #47761

GuillaumeGomez · 2018-01-25T23:47:25Z

QuietMisdreavus · 2018-01-26T00:21:35Z

[00:05:16] tidy error: /checkout/src/tools/rustdoc-themes/test-themes.py: incorrect license
[00:05:17] some tidy checks failed

bors · 2018-01-26T20:37:06Z

☔ The latest upstream changes (presumably #47748) made this pull request unmergeable. Please resolve the merge conflicts.

Mark-Simulacrum · 2018-01-26T21:57:20Z

src/bootstrap/check.rs

+        cmd.args(&["src/tools/rustdoc-themes/test-themes.py", rustdoc.to_str().unwrap()]);
+        cmd.env("RUSTC_STAGE", self.compiler.stage.to_string())
+           .env("RUSTC_SYSROOT", builder.sysroot(self.compiler))
+           .env("RUSTDOC_LIBDIR", builder.sysroot_libdir(self.compiler, builder.build.build))


This should probably be self.compiler.host as the second argument.

Mark-Simulacrum · 2018-01-26T21:58:09Z

src/bootstrap/check.rs

+    }
+
+    fn run(self, builder: &Builder) {
+        let rustdoc = builder.out.join("bootstrap/debug/rustdoc");


This should be a call to builder.rustdoc. Otherwise it may not have been built yet.

Mark-Simulacrum · 2018-01-26T21:58:46Z

src/bootstrap/check.rs

+
+    fn run(self, builder: &Builder) {
+        let rustdoc = builder.out.join("bootstrap/debug/rustdoc");
+        let mut cmd = Command::new(builder.config.python.clone().unwrap());


Please call expect("python not defined") or something similar.

Mark-Simulacrum · 2018-01-26T21:59:45Z

src/librustdoc/theme.rs

+    pathes.iter().cloned().collect()
+}
+
+pub fn load_css_pathes(v: &[u8]) -> CssPath {


nit: paths

Mark-Simulacrum · 2018-01-26T22:00:27Z

src/tools/rustdoc-themes/test-themes.py

+        print_err('No theme found in "{}"...'.format(THEME_DIR_PATH))
+        return 1
+    args = [rustdoc_bin, '-Z', 'unstable-options', '--theme-checker']
+    args.extend(themes)


So it's not super clear to me why we need this script instead of inlining this logic into rustbuild. That would be my preference...

It's simpler and more flexible from my point of view.

How is it better to have this in a python file vs having it in rustbuild? I prefer that we have as much as possible in rustbuild and in Rust. This is out-of-band and nearly impossible to find unless you know about it.

I'm going to side with @Mark-Simulacrum here. The fact that this script is in Python doesn't add anything over just having the logic inline with its call in rustbuild. It's literally just "for all the .css files in this folder except main.css, call rustdoc with these extra flags and that file". Python just seems to be invoked here for the directory-crawling aspect? You can get close enough with std::fs::read_dir, IMO.

GuillaumeGomez · 2018-01-27T22:48:39Z

@Mark-Simulacrum: Updated as you suggested.

mark-i-m · 2018-01-28T06:22:41Z

Cc rust-lang/rfcs#1987

QuietMisdreavus

I'm a little worried that this effectively contains a homebrew CSS parser. Would including a separate crate for that be too much of a problem?

QuietMisdreavus · 2018-01-29T21:53:38Z

src/librustdoc/lib.rs

+            print!(" - Checking \"{}\"...", theme_file);
+            let (success, differences) = theme::test_theme_against(theme_file, &paths);
+            if !differences.is_empty() || !success {
+                eprintln!(" FAILED");


I'm a little alarmed that the "Checking {}" and "FAILED" are sent to different output streams. Is this standard practice elsewhere?

Not sure. I'll give it a look.

QuietMisdreavus · 2018-01-29T22:50:15Z

src/librustdoc/theme.rs

+d {}
+"#;
+        let paths = load_css_paths(text.as_bytes());
+        assert!(paths.children.get(&CssPath::new("a b c d".to_owned())).is_some());


.get(..).is_some() is the same as .contains(..)

Oh indeed, thanks!

GuillaumeGomez · 2018-01-30T09:10:39Z

I'm a little worried that this effectively contains a homebrew CSS parser. Would including a separate crate for that be too much of a problem?

Not sure it's worth it. I mean, it's quite small and strongly integrated into rustdoc. This "CSS parser" is very basic and only get css-selectors by parsing comments and blocks.

GuillaumeGomez · 2018-02-03T10:30:56Z

So what should we do about this PR? We need it quickly if we want to add other themes (and to check those provided by users when they build docs with their own theme).

Mark-Simulacrum

I would like the python script removed, and then I think I'm mostly okay with landing this.

Mark-Simulacrum · 2018-01-30T18:15:02Z

src/librustdoc/theme.rs

+    pub children: HashSet<CssPath>,
+}
+
+// This PartialEq implementation IS NOT COMMUTATIVE!!!


This seems bad. Perhaps we shouldn't have a PartialEq (and much less Eq) impl and instead just have a method that does this? I'm somewhat concerned that we're going to run into problems with stdlib's HashSet etc where things oddly fail to work.

I appreciated to be able to use ==. 😞 I can do it in a method, but the basic issue will be the same: the check won't be commutative. We want to check that the second object implements all the first one's rules. If it has more, we don't care. We don't require an absolute equality. That's why I did it like this.

Hm. Yeah, I'd probably prefer a method (with a name like contains_rules or something), but I guess I don't care too much. Not a blocking concern, probably.

Mark-Simulacrum · 2018-01-30T18:15:17Z

src/librustdoc/theme.rs

+    }
+}
+
+impl Hash for CssPath {


This feels derivable?

No because to have Hash implemented for CssPath, you need to have it implemented on CssPath. :p

GuillaumeGomez · 2018-02-04T12:22:32Z

@Mark-Simulacrum: I'd prefer to keep the python script if possible for potential evolutions. For now it's quite simple but I have a few others things that'll be required later on with some new features. Having to update the builder rust code instead of having a small outside python script doesn't seem to be a very good idea from my point of view.

Mark-Simulacrum · 2018-02-04T15:37:05Z

I guess I'm failing to see why editing the Rust code is easier/better than the Python code. But I feel like we're somewhat talking past each other -- I don't think we've brought up any new arguments in this discussion for a while. Could we perhaps move the python code into Rust, and if later down the line we add additional features to it that make it easier to implement in Python, we can move it over again? I guess my main point here is that while Python may be easier for some (including perhaps you!) to edit and understand, I find Rust much easier to understand (and expect that to be true for the majority of Rust contributors). Does that help explain my reasoning for why it seems better to keep as much code as possible in Rust?

mark-i-m · 2018-02-04T19:17:12Z

In my limited experience, projects with multiple languages tend to be more work to maintain in the long run... IMHO, it would be good to keep everything in Rust...

GuillaumeGomez · 2018-02-04T19:34:49Z

Hum ok... So should I just replace the python code in rust or should I move this code directly into the builder?

QuietMisdreavus · 2018-02-04T20:16:29Z

Since you told me you want to extend this script in the future, i think it will be fine to just replace it with rust, without integrating it into rustbuild.

Mark-Simulacrum · 2018-02-04T21:44:05Z

Integrating it as a function in rustbuild would be easier as you don't have to worry about compiling it.

GuillaumeGomez · 2018-02-05T09:26:07Z

@Mark-Simulacrum: My only concern with this is that you don't know this test exists unless you wrote it. I like having the match between the test runner and a folder.

GuillaumeGomez · 2018-02-05T22:44:38Z

I converted the python script into rust.

Mark-Simulacrum

I'm mostly happy with this. r=me if you remove host in failure of just using compiler.host, otherwise I'd like to review again. Thanks for bearing with me!

Mark-Simulacrum · 2018-02-06T03:51:36Z

src/bootstrap/test.rs

+#[derive(Debug, Copy, Clone, Hash, PartialEq, Eq)]
+pub struct RustdocTheme {
+    pub compiler: Compiler,
+    pub host: Interned<String>,


Why is this not compiler.host? It should be afaict... or it should be named differently, or have some documentation. Right now this seems likely to be brittle in some configurations.

Mark-Simulacrum · 2018-02-06T03:52:33Z

src/librustdoc/theme.rs

+    pub children: HashSet<CssPath>,
+}
+
+// This PartialEq implementation IS NOT COMMUTATIVE!!!


Hm. Yeah, I'd probably prefer a method (with a name like contains_rules or something), but I guess I don't care too much. Not a blocking concern, probably.

Mark-Simulacrum · 2018-02-06T03:52:56Z

src/librustdoc/theme.rs

+    }
+}
+
+impl Hash for CssPath {


GuillaumeGomez · 2018-02-06T19:52:09Z

Updated.

GuillaumeGomez · 2018-02-08T09:55:42Z

@bors r=Mark-Simulacrum

bors · 2018-02-08T09:55:43Z

📌 Commit dec9fab has been approved by Mark-Simulacrum

bors · 2018-02-08T10:52:41Z

⌛ Testing commit dec9fab with merge ec9bf02...

@QuietMisdreavus

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors · 2018-02-08T13:53:57Z

💔 Test failed - status-appveyor

kennytm · 2018-02-08T14:03:08Z

@bors retry

3 hour timeout in check-aux x86_64-pc-windows-msvc and dist i686-pc-windows-gnu (stuck when cloning rust-by-example).

bors · 2018-02-08T16:55:05Z

⌛ Testing commit dec9fab with merge 4723336...

@QuietMisdreavus

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors · 2018-02-08T19:55:47Z

💔 Test failed - status-appveyor

kennytm · 2018-02-08T20:18:02Z

@bors retry (3 hour timeout, check x86_64-pc-windows-msvc)

This PR keeps timing out in some unconventional places.

GuillaumeGomez · 2018-02-08T20:56:03Z

All rust test chain is weird.

bors · 2018-02-09T08:24:00Z

⌛ Testing commit dec9fab with merge 02537fb...

@QuietMisdreavus

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors · 2018-02-09T11:17:59Z

☀️ Test successful - status-appveyor, status-travis
Approved by: Mark-Simulacrum
Pushing 02537fb to master...

rust-highfive assigned QuietMisdreavus Jan 25, 2018

kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 26, 2018

GuillaumeGomez force-pushed the test-themes branch 3 times, most recently from ab0eb13 to 6b716a6 Compare January 26, 2018 19:20

Mark-Simulacrum requested changes Jan 26, 2018

View reviewed changes

GuillaumeGomez force-pushed the test-themes branch 2 times, most recently from 819679f to 5083535 Compare January 27, 2018 16:46

QuietMisdreavus reviewed Jan 29, 2018

View reviewed changes

GuillaumeGomez force-pushed the test-themes branch from d8eb2f3 to 1cac26d Compare February 1, 2018 22:50

Mark-Simulacrum reviewed Feb 3, 2018

View reviewed changes

GuillaumeGomez force-pushed the test-themes branch from 1cac26d to 4c14d22 Compare February 5, 2018 22:44

GuillaumeGomez force-pushed the test-themes branch from 4c14d22 to f4be404 Compare February 5, 2018 22:45

Mark-Simulacrum reviewed Feb 6, 2018

View reviewed changes

GuillaumeGomez force-pushed the test-themes branch from f4be404 to ae657f5 Compare February 6, 2018 19:51

GuillaumeGomez force-pushed the test-themes branch from ae657f5 to dec9fab Compare February 8, 2018 09:53

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Feb 8, 2018

bors added a commit that referenced this pull request Feb 8, 2018

Auto merge of #47761 - GuillaumeGomez:test-themes, r=Mark-Simulacrum

ec9bf02

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 8, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2018

bors added a commit that referenced this pull request Feb 8, 2018

Auto merge of #47761 - GuillaumeGomez:test-themes, r=Mark-Simulacrum

4723336

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 8, 2018

bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 8, 2018

bors added a commit that referenced this pull request Feb 9, 2018

Auto merge of #47761 - GuillaumeGomez:test-themes, r=Mark-Simulacrum

02537fb

Test themes r? @QuietMisdreavus cc @Mark-Simulacrum

bors merged commit dec9fab into rust-lang:master Feb 9, 2018

bors mentioned this pull request Feb 9, 2018

WIP: Parallelize passes using rayon #46564

Closed

GuillaumeGomez deleted the test-themes branch February 9, 2018 12:40

kennytm mentioned this pull request Mar 10, 2018

Use free regions when determining self type in compare_impl_method #48419

Merged

ehuss mentioned this pull request Aug 14, 2020

Fix crate-version with rustdoc in bootstrap. #75539

Merged

Test themes #47761

Test themes #47761

Conversation

GuillaumeGomez commented Jan 25, 2018

QuietMisdreavus commented Jan 26, 2018

bors commented Jan 26, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 27, 2018

mark-i-m commented Jan 28, 2018

QuietMisdreavus left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 30, 2018

GuillaumeGomez commented Feb 3, 2018

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 4, 2018

Mark-Simulacrum commented Feb 4, 2018

mark-i-m commented Feb 4, 2018

GuillaumeGomez commented Feb 4, 2018

QuietMisdreavus commented Feb 4, 2018

Mark-Simulacrum commented Feb 4, 2018

GuillaumeGomez commented Feb 5, 2018

GuillaumeGomez commented Feb 5, 2018

Mark-Simulacrum left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

GuillaumeGomez commented Feb 6, 2018

GuillaumeGomez commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 8, 2018

kennytm commented Feb 8, 2018

bors commented Feb 8, 2018

bors commented Feb 8, 2018

kennytm commented Feb 8, 2018

GuillaumeGomez commented Feb 8, 2018

bors commented Feb 9, 2018

bors commented Feb 9, 2018