Closed Bug 1491799 Opened 1 year ago Closed 1 year ago

Refactor geckodriver build script

Categories

(Testing :: geckodriver, enhancement, P1)

enhancement

Tracking

(firefox64 fixed)

RESOLVED FIXED
mozilla64
Tracking Status
firefox64 --- fixed

People

(Reporter: ato, Assigned: ato)

References

Details

Attachments

(1 file, 1 obsolete file)

This is to refactor the geckodriver build script so we can provide
VCS specific instructions.
Depends on: 1491408
Assignee: nobody → ato
Status: NEW → ASSIGNED
Priority: -- → P1
Blocks: 1491801
Blocks: 1491802
Provides almost the same functionality, but without convoluted
functional style.  This will allow us to provide VCS specific
instructions in the future.
Attachment #9009620 - Flags: review?(hskupin)
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?
Attachment #9009620 - Flags: review?(hskupin) → review-
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.
Provides almost the same functionality, but without convoluted
functional style.  This will allow us to provide VCS specific
instructions in the future.
Attachment #9010202 - Flags: review?(hskupin)
Attachment #9009620 - Attachment is obsolete: true
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?
Attachment #9010202 - Flags: review?(hskupin) → review+
Pushed by ato@sny.no:
https://hg.mozilla.org/integration/mozilla-inbound/rev/d059d18bbe2c
geckodriver: refactor build script; r=whimboo
https://hg.mozilla.org/mozilla-central/rev/d059d18bbe2c
Status: ASSIGNED → RESOLVED
Closed: 1 year ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla64
You need to log in before you can comment on or make changes to this bug.