1491799 - Refactor geckodriver build script

Status: RESOLVED FIXED

(Testing :: geckodriver, enhancement, P1)

Description

[Andreas Tolfsen ❲:ato❳]

This is to refactor the geckodriver build script so we can provide
VCS specific instructions.

Comment 1

[Andreas Tolfsen ❲:ato❳]

Attachment: geckodriver: refactor build script

Provides almost the same functionality, but without convoluted
functional style.  This will allow us to provide VCS specific
instructions in the future.

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 9009620 [details] [diff] [review]
geckodriver: refactor build script

Review of attachment 9009620 [details] [diff] [review]:
-----------------------------------------------------------------

::: testing/geckodriver/build.rs
@@ +18,4 @@
>  use std::process::Command;
>  
> +fn main() -> io::Result<()> {
> +    let topsrcdir = PathBuf::from(env::var("CARGO_MANIFEST_DIR").unwrap());

This is not returning the topsrcdir from the repository, but "The directory containing the manifest of your package.". As such it is `testing/geckodriver/`, and we shouldn't name it that way.

@@ +43,5 @@
> +    } else if Path::exists(&dir.join(".git")) {
> +        Box::new(Git {})
> +    } else {
> +        if let Some(parent) = dir.parent() {
> +            get_build_info(parent)

Note that this escapes the root folder of the repository in case it is a download of the packed archive. But I don't know if we expect the command to work under such a condition.

@@ +64,5 @@
> +    where
> +        I: IntoIterator<Item = S>,
> +        S: AsRef<OsStr>,
> +    {
> +        Command::new("hg")

I don't see the changes from bug 1491408 in that patch.

@@ +68,5 @@
> +        Command::new("hg")
> +            .args(args)
> +            .output()
> +            .ok()
> +            .and_then(|r| String::from_utf8(r.stdout).ok())

No need to trim anymore?

@@ +90,5 @@
> +    where
> +        I: IntoIterator<Item = S>,
> +        S: AsRef<OsStr>,
> +    {
> +        Command::new("git")

Same as for Hg, I miss the changes from bug 1491408.

@@ +94,5 @@
> +        Command::new("git")
> +            .args(args)
> +            .output()
> +            .ok()
> +            .and_then(|r| String::from_utf8(r.stdout).ok())

Why aren't we converting to a hg changeset anymore?

Comment 3

[Andreas Tolfsen ❲:ato❳]

Thanks for pointing out a few glaring omissions.  I’ve addressed
all of them in the latest patch.

(In reply to Henrik Skupin (:whimboo) from comment #2)

> @@ +43,5 @@
> > +    } else if Path::exists(&dir.join(".git")) {
> > +        Box::new(Git {})
> > +    } else {
> > +        if let Some(parent) = dir.parent() {
> > +            get_build_info(parent)
> 
> Note that this escapes the root folder of the repository in case
> it is a download of the packed archive. But I don't know if we
> expect the command to work under such a condition.

Yes, that is the expected behaviour.  It returns a Noop VCS in this
case, which provide None for BuildInfo::hash() and ::date(), which
is the same behaviour as the old build script.

Comment 4

[Andreas Tolfsen ❲:ato❳]

Attachment: geckodriver: refactor build script

Provides almost the same functionality, but without convoluted
functional style.  This will allow us to provide VCS specific
instructions in the future.

Comment 5

[Henrik Skupin [:whimboo][⌚️UTC+2] (away July 24th - August 16th)]

Comment on attachment 9010202 [details] [diff] [review]
geckodriver: refactor build script

Review of attachment 9010202 [details] [diff] [review]:
-----------------------------------------------------------------

From my knowledge of Rust this looks fine. Maybe James could have another look at it?

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d059d18bbe2c
geckodriver: refactor build script; r=whimboo

Comment 7

[Bogdan Tara[:bogdan_tara | bogdant]]

https://hg.mozilla.org/mozilla-central/rev/d059d18bbe2c