-
Notifications
You must be signed in to change notification settings - Fork 557
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
feat: use jsdiff instead of 'patch' to reduce dependency on OS binaries #355
Conversation
37e9ac6
to
655537e
Compare
I tested this manually by applying the We will huddle internally on how to best move forward in the least risky way. Really excited about this opportunity, thanks again @saschanaz ! |
Hi @saschanaz, I just wanna let you know we haven't forgotten your awesome contribution and we've proceed a bit more with internal testing and we hope to merge this PR soon. Thank you again for your help! |
@@ -1,6 +1,7 @@ | |||
module.exports = applyPatch; | |||
|
|||
var debug = require('debug')('snyk'); | |||
var diff = require('diff'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we added the @types pkg for it, so why not .ts
it? 😉
package.json
Outdated
@@ -51,11 +51,13 @@ | |||
"dependencies": { | |||
"@snyk/dep-graph": "1.1.2", | |||
"@snyk/gemfile": "1.1.0", | |||
"@types/diff": "^3.5.2", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
why this, and the other types are actually here and not in devDependencies
? 🤔
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yep - lodash
& node
types are in devDependencies
so all these are here by mistake
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll move this.
if (error || out.indexOf('FAILED') !== -1) { | ||
debug('patch command failed', relative, error, out); | ||
return patchError(error, out, relative, vuln).catch(reject); | ||
function jsDiff(patchContent, relative, live) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
what is live
? I see we had such a var before, but maybe it's a chance to improve the naming?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
live
is a boolean that decides whether the patch result should be written to the file system or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Cool, thanks. Would suggest renaming it to isDryRun
(and reversing the logic checks 😅).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This goes way up into the flow (https://github.com/snyk/snyk/blob/master/src/cli/commands/protect/index.js#L78), used in wizard
flow too. Suggest to change this on a separate chore
PR.
src/lib/protect/apply-patch.js
Outdated
diff.applyPatches(patchContent, { | ||
loadFile: function (index, callback) { | ||
try { | ||
var fileName = stripFirstSlash(index.oldFileName); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
would suggest add comment with a sample path(s), to illustrate why we do this.
src/lib/protect/apply-patch.js
Outdated
debug('patch command failed', relative, error, out); | ||
return patchError(error, out, relative, vuln).catch(reject); | ||
function jsDiff(patchContent, relative, live) { | ||
const files = {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
maybe rename to patchedFiles
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Doing this.
Force-pushed after rebasing over |
@michael-go can you please ✅ so I can squash the commits and merge this? ✨ |
🎉 This PR is included in version 1.131.0 🎉 The release is available on: Your semantic-release bot 📦🚀 |
@saschanaz 👋 I was wondering if you could shed some light on the situation as we see it. Sometimes, we end up processing an empty hunk. This code runs - https://github.com/snyk/snyk/blob/49dc318db8a5e46bb324c10909239d954cdd3217/src/lib/protect/apply-patch.js#L46-L59 - with the This is happening for our users here and there, but we are not able to reproduce the issue locally. Any thoughts about this would be greatly appreciated 🙏 |
Looks like it may happen when a patch file has no header. Is there an headerless patch on snyk? |
Hmmm Not only do we not have headerless patches, but this is also not persistent at all in terms of reproduction. Could this be a race condition when we have yet created the patch file locally, and are already trying to read it? Will check! |
if (patchedFiles[fileName] === null) { | ||
fs.unlinkSync(path.resolve(relative, fileName)); | ||
} | ||
fs.writeFileSync(path.resolve(relative, fileName), patchedFiles[fileName]); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This line looks like a potential bug generator where null
would trigger removal and then rewrite.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This area writes the patched dependency source back to the disk, seems ok :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The value null
was intended to delete the file (hence the unlink) but here it instead overwrites with the text null
.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Aha! I see your point. Will make this safer.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
See 908bc3a
@saschanaz thanks for your assistance here! I think we found the culprit - indeed a race condition where we We're going to fix this now, have been carrying this problem with us for a very long time! 😌 |
What does this PR do?
Applying patches on
snyk protect
requires thepatch
binary to be available. This is problematic especially on Windows envs where GNU binaries are not installed by default.With the help of @saschanaz in #335 we're giving
jsdiff
another go, with the intent to remove the dependency on thepatch
binary, and apply patches with pure JS.