feat(capture_webrender): write webrender revision into text #20320

kwonoj · 2018-03-16T21:24:14Z

Relates to #20315 (comment).

This PR try to generate wr.txt when trigger webrender capture. By reading gecko's implementation at here, it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing.

In this PR tries to similar in cruxwise with small differences:

cargo build reads cargo.lock, export it into ${OUT_DIR}/, included via macro in build time.
when capturing triggered, those revision will be written as wr.txt.

Probably point of discussion & need to be updated in PR if necessary:
~~- Is it acceptable mach generates module file on build bootstrapping? Should there be other recommendation?~~ Now cargo build takes care of generation.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix Add hotkey for WebRender capturing #20295 (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____
This PR manually verified on local mac OS machine.

This change is

highfive · 2018-03-16T21:24:19Z

Heads up! This PR modifies the following files:

@wafflespeanut: python/servo/command_base.py
@paulrouget: components/compositing/lib.rs, components/compositing/compositor.rs

kwonoj · 2018-03-16T21:26:16Z

/cc @kvark from prior discussion in #20315

jdm · 2018-03-16T21:27:45Z

Unfortunately https://github.com/servo/webrender#0.57.0 doesn't include the git revision, so it's not precise enough for this.

kwonoj · 2018-03-16T21:29:40Z

~~@jdm I assume so. so using cargo.lock is also no-go, I guess?~~

kwonoj · 2018-03-16T21:31:16Z

Let me try to explore if there's way to grab commit sha when bootstrapping.

jdm · 2018-03-16T21:33:43Z

cargo.lock contains the revision:

servo/Cargo.lock

Line 3484 in 74d6a91

    
           source = "git+https://github.com/servo/webrender#fefb2820f15d733cbddfcdac60ef10f616e83cc7"

kwonoj · 2018-03-16T21:35:41Z

My bad, seems I wasn't correctly searching lockfile. Appreciate for tip, let me try to update PR using it.

kwonoj · 2018-03-16T22:01:58Z

@jdm appreciate for pointing, PR now updated to include sha from lockfile.

kvark · 2018-03-17T02:48:53Z

.gitignore

@@ -23,6 +23,7 @@ Servo.app
 .config.mk.last
 /glfw
 capture_webrender/
+/components/compositing/webrender_revision.rs


Is there a particular reason for routing the revision through rust code? could just generate a "wr.txt" and just copy it, alternatively

Yes, I mentioned in PR body if this is an acceptable approach. I wasn't entirely sure about copying text file for cases if user downloaded nightly build wanted to capture it, does lookup generated text file would works with same path resolution to dev build? Thought having constant in build itself will makes those always working.

Sorry, I don't understand the concern "does lookup generated text file would works with same path resolution to dev build". Please rephrase?

I guess it's my misunderstanding mostly. in short, does packaged nightly build can lookup where wr.txt same as mach run on dev build?

oh, I see. So it's a question to package builders on how to make sure "wr.txt" is included. @jdm do you know who forward this to?

kvark · 2018-03-17T02:54:44Z

As it appears to me, the generated "wr.txt" contains the full repo path in addition to the revision. FYI, this is different from Gecko-generated file which only has the SHA. Not a blocker, just wondering if this is intentional.

kwonoj · 2018-03-17T02:56:28Z

if this is intentional.

Not an intentional, mostly cause just grabbed source from parsed lockfile without taking out repo source. I think it's totally doable, will apply those changes in PR.

(edited): PR updated to grab sha only.

nox · 2018-03-19T09:30:32Z

components/compositing/compositor.rs

+                let revision_file_path = capture_path.join("wr.txt");
+
+                if let Err(err) = create_dir_all(&capture_path) {
+                    println!("Unable to create path '{:?}' for capture: {:?}", capture_path, err);


nox · 2018-03-19T14:16:56Z

Why isn't WR itself putting its revision in a constant in its public API? Cc @kvark @nical

kvark · 2018-03-19T14:21:12Z

@nox because nobody bothered to explore this path yet. In Gecko, the revision was already tracked and maintained in a text file, so all I had to do is move it into a separate file and copy into a capture when requested. If the logic required in Servo is equivalent to just doing it in WR upstream, then let's move it to WR and then we'll patch Gecko to avoid redundant revision tracking.

nox · 2018-03-19T14:23:50Z

Putting it in the public API of WR seems better to me. What do you think?

kwonoj · 2018-03-19T14:25:21Z

If WR exports it, I think consumer(servo) would be easier to read it. Guess I can close this PR?

kvark · 2018-03-19T14:28:07Z

@nox wait. Gecko vendors WR dependency, so the build.rs will not be able to extract the actual WR version from the cargo lock...

nox · 2018-03-19T14:30:58Z

@kvark Ok.

In any case, I'm 0- for what is suggested here. Please use a build script to generate the Rust constant that will hold the revision, let's not have to explicitly run mach to generate it.

kvark · 2018-03-19T14:37:30Z

@nox to clarify, "0-" means you are not objecting but not so fond of the change either?

Please use a build script to generate the Rust constant that will hold the revision, let's not have to explicitly run mach to generate it.

So you don't want to keep track of a separate file and consider the current approach to be good (minus the "eprintln" nit)? I'm fine with that.

nox · 2018-03-19T14:39:36Z

@nox to clarify, "0-" means you are not objecting but not so fond of the change either?

Yes.

So you don't want to keep track of a separate file and consider the current approach to be good (minus the "eprintln" nit)? I'm fine with that.

My issue isn't about the separate file, but the fact that the generation is made from mach, which means calling cargo build directly will probably break, AFAICT.

kwonoj · 2018-03-23T04:46:19Z

Updated per review suggestion and rebased to latest master. Let me know anytime if there's thing should followup.

jdm

Apart from my question about alternate build configurations, this implementation looks solid to me.

jdm · 2018-03-23T05:26:56Z

components/compositing/build.rs

+                        .iter()
+                        .find(|pkg| pkg.get("name").and_then(|name| name.as_str()).unwrap_or("") == "webrender")
+                        .and_then(|pkg| pkg.get("source").and_then(|source| source.as_str()))
+                        .expect("webrender package does not conatin source to read revision");


Does the build still work if you create an override for webrender in Cargo.toml?

[patch."https://github.com/servo/webrender"] webrender = { path = "../webrender/webrender" }

I just tried locally by specifying above section in components/composition/cargo.toml, seems build success without failure. Not sure if I tried correct way to validate this.

It belongs in the Cargo.toml in the root of the repository instead.

My bad, thanks for let me know.

Just tried and build fails with overriding, as updated cargo lock omits source property to read revision information.

I think in that case we should let the build proceed with a warning that the webrender revision could not be determined, and store a value like <unknown>.

agreed, I'll try to update implementation to reflect those cases. Appreciate for let me know.

kwonoj · 2018-03-23T06:22:10Z

build.rs is updated to allow overridden package doesn't have source property with revision - in that case it'll fall back to unknown string instead.

jdm · 2018-03-23T06:30:46Z

@bors-servo: r+
Thanks!

bors-servo · 2018-03-23T06:30:47Z

📌 Commit 5f69773 has been approved by jdm

bors-servo · 2018-03-23T06:30:51Z

⌛ Testing commit 5f69773 with merge 91398cf...

feat(capture_webrender): write webrender revision into text  Relates to #20315 (comment). This PR try to generate `wr.txt` when trigger webrender capture. By reading gecko's implementation at [here](https://github.com/mozilla/gecko-dev/blob/3b8e63c66ae1989cfc2c7fb48ca9e025a3828e74/gfx/doc/README.webrender#L53), it seems gecko's build script generates txt file for containing revision of webrender and read it each time trigger capturing. In this PR tries to similar in cruxwise with small differences: - `cargo build` reads `cargo.lock`, export it into `${OUT_DIR}/`, included via macro in build time. - when capturing triggered, those revision will be written as `wr.txt`. Probably point of discussion & need to be updated in PR if necessary: ~- Is it acceptable `mach` generates module file on build bootstrapping? Should there be other recommendation?~ Now cargo build takes care of generation. ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #20295 (github issue number if applicable).  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because _____ - This PR manually verified on local mac OS machine.    --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/20320)

bors-servo · 2018-03-23T07:29:14Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css1, mac-rel-css2, mac-rel-wpt1, mac-rel-wpt2, mac-rel-wpt3, mac-rel-wpt4, windows-msvc-dev
Approved by: jdm
Pushing 91398cf to master...

highfive assigned asajeffrey Mar 16, 2018

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 16, 2018

kwonoj changed the title ~~feat(capture_webrender): write webrender revision into text Verified 79e2ebb~~ feat(capture_webrender): write webrender revision into text Mar 16, 2018

kwonoj force-pushed the feat-wr-revision branch from 79e2ebb to 5c5c237 Compare March 16, 2018 22:00

kwonoj force-pushed the feat-wr-revision branch 3 times, most recently from f76eabb to 56b9d7e Compare March 16, 2018 22:33

kvark reviewed Mar 17, 2018

View reviewed changes

kwonoj force-pushed the feat-wr-revision branch 3 times, most recently from 1e78da6 to 6287a95 Compare March 19, 2018 01:24

nox reviewed Mar 19, 2018

View reviewed changes

kwonoj force-pushed the feat-wr-revision branch from 1926571 to 5218910 Compare March 19, 2018 21:49

kwonoj added 2 commits March 19, 2018 15:47

build(mach): generate webrender revision via cargo lockfile

6b16e71

feat(capture_webrender): write webrender revision into text

0671fcb

kwonoj force-pushed the feat-wr-revision branch 4 times, most recently from 6453d9f to 1818f00 Compare March 23, 2018 04:45

jdm approved these changes Mar 23, 2018

View reviewed changes

jdm added S-awaiting-answer Someone asked a question that requires an answer. and removed S-awaiting-review There is new code that needs to be reviewed. labels Mar 23, 2018

kwonoj added 3 commits March 22, 2018 23:20

refactor(build): generate revision via build.rs

f923097

refactor(build): generate revision into out_dir

b957f6a

style(capture_webrender): use eprintln for error output

5f69773

kwonoj force-pushed the feat-wr-revision branch from 1818f00 to 5f69773 Compare March 23, 2018 06:20

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Mar 23, 2018

highfive assigned jdm and unassigned asajeffrey Mar 23, 2018

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. S-awaiting-answer Someone asked a question that requires an answer. labels Mar 23, 2018

bors-servo merged commit 5f69773 into servo:master Mar 23, 2018

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Mar 23, 2018

kwonoj deleted the feat-wr-revision branch March 24, 2018 06:12

feat(capture_webrender): write webrender revision into text #20320

feat(capture_webrender): write webrender revision into text #20320

Conversation

kwonoj commented Mar 16, 2018 • edited

highfive commented Mar 16, 2018

kwonoj commented Mar 16, 2018

jdm commented Mar 16, 2018

kwonoj commented Mar 16, 2018 • edited

kwonoj commented Mar 16, 2018

jdm commented Mar 16, 2018

kwonoj commented Mar 16, 2018

kwonoj commented Mar 16, 2018 • edited

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

kvark commented Mar 17, 2018

kwonoj commented Mar 17, 2018 • edited

Choose a reason for hiding this comment

nox commented Mar 19, 2018

kvark commented Mar 19, 2018 • edited

nox commented Mar 19, 2018

kwonoj commented Mar 19, 2018

kvark commented Mar 19, 2018

nox commented Mar 19, 2018

kvark commented Mar 19, 2018

nox commented Mar 19, 2018

kwonoj commented Mar 23, 2018

jdm 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

kwonoj commented Mar 23, 2018

jdm commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

bors-servo commented Mar 23, 2018

kwonoj commented Mar 16, 2018 •

edited

kwonoj commented Mar 16, 2018 •

edited

kwonoj commented Mar 16, 2018 •

edited

kwonoj commented Mar 17, 2018 •

edited

kvark commented Mar 19, 2018 •

edited