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)
Details
Details
Run yarn cleaninstall and see that patch-package is run at the end
Diff Detail
Diff Detail
- Repository
- rCOMM Comm
- Lint
No Lint Coverage - Unit
No Test Coverage
Event Timeline
Comment Actions
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.
Comment Actions
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 | 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 | Let's avoid copy-pasting this line if possible | |
Comment Actions
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).
Comment Actions
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? |
Comment Actions
Looks ok to me
| keyserver/Dockerfile | ||
|---|---|---|
| 27–36 ↗ | (On Diff #21664) | Why this formatting is changed? |
Comment Actions
Note that I accepted above with comments – see my comments above before landing please