Page MenuHomePhabricator

[services][electron update server] Add patch-package to root postinstall
ClosedPublic

Authored by michal on Jan 25 2023, 7:31 AM.
Tags
None
Referenced Files
Unknown Object (File)
Thu, Dec 19, 4:36 PM
Unknown Object (File)
Thu, Dec 19, 4:09 PM
Unknown Object (File)
Wed, Dec 18, 6:13 PM
Unknown Object (File)
Wed, Dec 18, 6:10 PM
Unknown Object (File)
Wed, Dec 18, 6:03 PM
Unknown Object (File)
Wed, Dec 18, 5:41 PM
Unknown Object (File)
Wed, Dec 18, 4:53 PM
Unknown Object (File)
Wed, Dec 18, 2:34 PM
Subscribers

Details

Summary

Currently patch-package is only run from the native workspace but it would make sense to run it at the end after all workspace members are correctly installed (in particular the new electron-update-server requires patches to the hazel package)

Test Plan

Run yarn cleaninstall and see that patch-package is run at the end

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 25 2023, 7:34 AM
Harbormaster failed remote builds in B15639: Diff 21320!

Rebase. In the native's postinstall.sh also create olm/package.json when building on windows. Otherwise it would fail when we run patch-package from root. The command is different because in Windows when the scripts goes up a directory it goes from node_modules/native comm/node_modules instead of comm/. This doesn't happen on macOS/ linux because comm/node_modules/native is symlinked to comm/native.

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 30 2023, 5:59 AM
Harbormaster failed remote builds in B15845: Diff 21610!

Try moving the patch-package to depDependencies

Harbormaster returned this revision to the author for changes because remote builds failed.Jan 30 2023, 6:43 AM
Harbormaster failed remote builds in B15847: Diff 21612!

Try calling native postinstall in root postinstall

Ok, I'm not sure why that fixed CI. Not sure if this is the best solution, but I was just trying things. Here are some failed builds from previous revisions: buildkite1, buildkite2, buildkite3.

Some more context: native uses patch-package to patch olm but it's a git dependency and doesn't have a package.json and patch-package fails. So in native postinstall script, we add a basic package.json to olm before patching.
The new root postinstall fails because it doesn't seem to find the olm/package.json. Not sure why that happens, postinstall in workspace root should run after the members'.

native/postinstall.sh
1 ↗(On Diff #21618)

Does this script running the same working directory for both native and root patch-package?

I wonder if it would better to not run it in the root patch-package, and instead run it from the workspaces that need it (eg. native and electron-update-server)

7 ↗(On Diff #21618)

Let's avoid copy-pasting this line if possible

Try moving postinstall from native to root.

Fix keyserver dockerfile


Moving postinstall from native to root seems to work. I think it's better than:

I wonder if it would better to not run it in the root patch-package, and instead run it from the workspaces that need it (eg. native and electron-update-server)

because it's only running once (and also it's more in line with what was assumed before, that postinstall only runs after all dependencies are installed).

This is a big change and I worry that it will break some workflows. For instance, are we sure that the postinstall script in the root will run after all of the packages in native are downloaded and installed?

Accepting to unblock, but please make sure to monitor GitHub Actions CI after landing, and revert this if it breaks.

keyserver/Dockerfile
27–36 ↗(On Diff #21664)

Please undo these changes, and in the future review your diffs before submitting them (eg. with git diff, or at least looking at it on Phabricator) to make sure you're not introducing changes like this

postinstall.sh
12 ↗(On Diff #21664)

This package is in native, not in the root... should it be moved to the root?

This revision is now accepted and ready to land.Jan 31 2023, 6:27 AM
tomek added 1 blocking reviewer(s): ashoat.

Looks ok to me

keyserver/Dockerfile
27–36 ↗(On Diff #21664)

Why this formatting is changed?

This revision now requires review to proceed.Jan 31 2023, 6:31 AM

Note that I accepted above with comments – see my comments above before landing please

This revision is now accepted and ready to land.Jan 31 2023, 6:32 AM

Revert accidental formatting changes.