1757701 - "mach puppeteer-test" fails with ".git can't be found (see https://git.io/Jc3F9)"

Status: RESOLVED FIXED

(Remote Protocol :: Agent, defect, P3)

Description

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I'm currently not able to run mach puppeteer-test locally because by default Puppeteer gets installed first and some step requires husky. But our vendored in code doesn't contain the .git folder, and as such npm prepare fails.

With the recently added --no-install option (bug 1757523) I can at least circumvent this problem for now.

Here the detailed failure:

✗ mach puppeteer-test -vv --subset                                                                                                                                                  git:(main↓163|✚1…3

> puppeteer@13.0.1 install
> node install.js

**INFO** Skipping browser download. "PUPPETEER_SKIP_DOWNLOAD" environment variable was found.

> puppeteer@13.0.1 prepare
> node typescript-if-required.js && husky install

.git can't be found (see https://git.io/Jc3F9)
npm ERR! code 1
npm ERR! path /Users/henrik/code/gecko/remote/test/puppeteer
npm ERR! command failed
npm ERR! command sh -c node typescript-if-required.js && husky install

npm ERR! A complete log of this run can be found in:
npm ERR!     /Users/henrik/.npm/_logs/2022-03-02T08_00_27_571Z-debug.log
/Users/henrik/code/gecko/mach: npm: exit code 1

Julian mentioned that he had similar issues when vendoring the Puppeteer 13.0.1 release (bug 1732958).

Given that we do not need husky it would be good to know what to change to prevent this failure.

Comment 1

[Julian Descottes [:jdescottes]]

I think my workaround was to use --ignore-scripts https://docs.npmjs.com/cli/v8/commands/npm-install#ignore-scripts
It will skip post/pre scripts for the install. But it still skips the other bit from prepare: node typescript-if-required.js.

Husky is only ever useful if you are using the repo to commit changes back, whereas the one we have in central is only a "vendored".

We could introduce a new env variable and update the package.json in order to check it before installing husky. Will require a sync with the puppeteer repo though.

Eg: https://hg.mozilla.org/try/rev/ae68ae6b3d3b88a817db00004e0e8ae51ebd5c00

Henrik: what do you think?

Comment 2

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Hm, I don't think that we might get that change through their review process. Instead I would suggest that we just update package.json when vendoring in Puppeteer and remove the husky part. If that's ok with you we should update our docs as well.

Comment 3

[Julian Descottes [:jdescottes]]

Fair enough, we could still ask and see, but I'm fine with a manual update of package.json

Comment 4

[Henrik Skupin [:whimboo][⌚️UTC+1]]

I talked to Alex and he pointed me to: https://typicode.github.io/husky/#/?id=bypass-hooks

Even with the HUSKY environment variable not working, I was able to set a custom prepare script so that we run node typescript-if-required.js. This seems to be required anyway, and following code will fail if resetting the prepare script.

As such a possible solution could look like:

@@ -695,16 +696,18 @@ def install_puppeteer(command_context, product, ci):

     if product != "chrome":
         env["PUPPETEER_SKIP_DOWNLOAD"] = "1"
     lib_dir = os.path.join(command_context.topsrcdir, puppeteer_dir, "lib")
     if changed_files and os.path.isdir(lib_dir):
         # clobber lib to force `tsc compile` step
         shutil.rmtree(lib_dir)

+    npm("set-script", "prepare", "node typescript-if-required.js")
+
     command = "ci" if ci else "install"

Comment 5

[BugBot [:suhaib / :marco/ :calixte]]

The severity field is not set for this bug.
:whimboo, could you have a look please?

For more information, please visit auto_nag documentation.

Comment 6

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It turns out that we cannot actually use the set-script command because that modifies the global package.json file in our repository. So we may have to modify the Puppeteer's package.json and remove husky whenever we vendor in the code. Will check back with Alex for possible alternatives.

Comment 7

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Note that this issue is not visible when using Mercurial as VCS for working with mozilla-central.

Comment 8

[Henrik Skupin [:whimboo][⌚️UTC+1]]

(In reply to Henrik Skupin (:whimboo) [⌚️UTC+1] from comment #7)

Note that this issue is not visible when using Mercurial as VCS for working with mozilla-central.

Mitchell, could this issue maybe related with mach and it's git wrapper? At least it feels so given that it's not visible when using Mercurial.

Comment 9

[Mitchell Hentges [:mhentges] 🦀]

Mitchell, could this issue maybe related with mach and it's git wrapper?

FWIW, Mach doesn't wrap git, but there's something spicy going on if this is only reproducible on git-cloned trees.
I'd recommend investigating this by diffing mach puppeteer-test -vv --subset in an hg and git clone.

Comment 10

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Actually I was on a hg branch that had a patch applied, which I was experimenting with a while ago. Then I switched to git and forgot to change the branch to mozilla-central again. That means the wanted patch actually works and I'll go ahead and propose that for us.

As first step I've created an upstream PR to allow us to not install Husky if the HUSKY=0 environment variable has been set:
https://github.com/puppeteer/puppeteer/pull/8325

Once approved and merged I'll continue with the modifications for our downstream sync.

Comment 11

[Henrik Skupin [:whimboo][⌚️UTC+1]]

It's a clean-up which fixes a breakage when trying to run Puppeteer tests and not passing in --no-install. As such it would be great to see it fixed in the current milestone.

Comment 12

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1757701 - [puppeteer] Allow to skip the installation of Husky in the prepare step.

Comment 13

[Henrik Skupin [:whimboo][⌚️UTC+1]]

Attachment: Bug 1757701 - [puppeteer] Disable Husky when installing Puppeteer via npm.

Depends on D145857

Comment 14

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/autoland/rev/eb59cb42fae0
[puppeteer] Allow to skip the installation of Husky in the prepare step. r=webdriver-reviewers,jdescottes
https://hg.mozilla.org/integration/autoland/rev/69462d07ca4a
[puppeteer] Disable Husky when installing Puppeteer via npm. r=webdriver-reviewers,jdescottes

Comment 15

[Atila Butkovits]

https://hg.mozilla.org/mozilla-central/rev/eb59cb42fae0
https://hg.mozilla.org/mozilla-central/rev/69462d07ca4a