Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

When a patch fails during an upgrade, the error log is too hard to understand #361

Closed
estellecomment opened this issue Jan 9, 2023 · 6 comments
Labels
devops P3 Priority 3

Comments

@estellecomment
Copy link
Contributor

estellecomment commented Jan 9, 2023

When we upgrade element-web, and run "yarn install", the postinstall runs after yarn install, which applies patches. We expect some patches to break, this is normal since matrix-react-sdk and matrix-js-sdk can have changed.
In this case, we want the postinstall to crash and explain why it is crashing.

Expected :
The log explains clearly what happened : the break comes from patch-package, and the patch that failed is .

Obtained :
The log messages from "apply_patches.sh" are missing, which makes the error hard to track down.

Notes :

  • the process to figure it out is to remove the postinstall from package.json, so that yarn install runs fully (without postinstall) and then run postinstall separately, which gives understandable errors.
  • This is probably because the crash on error is "dirty", it doesn't exit cleanly and empty its log buffers ? Is this coming from "--exit-on-fail" ?
  • We do need postinstall to crash, because we want to know there is a problem. Removing the crash completely is not a solution.
@odelcroi odelcroi added P2 Priority 2 devops labels Mar 24, 2023
@jdauphant
Copy link
Contributor

@odelcroi with the new script merge_patchs.sh , I think we can close this issue

@odelcroi
Copy link
Member

when --exit-on-fail is removed, the patch fails silently but the log is not easier to read

@odelcroi
Copy link
Member

@odelcroi with the new script merge_patchs.sh , I think we can close this issue

No, this error happens when applying the patch, not merging them

@odelcroi odelcroi added P3 Priority 3 and removed P2 Priority 2 labels May 12, 2023
@estellecomment
Copy link
Contributor Author

One cause is that patch-package is not logging the right patch-dir, so we cannot tell which patch has errored.
I submitted the fix to patch-package : ds300/patch-package#488

@estellecomment
Copy link
Contributor Author

... and the corresponding fix for tchap-web : #683

This is a partial fix : postinstall seems to eat the logs in various ways, and sometimes it will output the patch-package log, in which case this fix will help (we'll get the right info from the log). When all logs are eaten by postinstall, it doesn't help.

(a better fix is to stop postinstall from eating logs...)

@estellecomment
Copy link
Contributor Author

looks like #684 is a fix ! 🥳

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
devops P3 Priority 3
Projects
None yet
Development

No branches or pull requests

3 participants