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

Delete superfluous .patch files. #1122

Merged
merged 1 commit into from
Mar 20, 2017
Merged

Delete superfluous .patch files. #1122

merged 1 commit into from
Mar 20, 2017

Conversation

bnoordhuis
Copy link
Member

The patch files are unnecessary, they also exist as commits in the
history. Delete the files.

Refs: #1119

@refack
Copy link
Contributor

refack commented Feb 16, 2017

@bnoordhuis but they make rebasing easier... 😞

@bnoordhuis
Copy link
Member Author

Why? You can just git cherry-pick the commits after an update.

@refack
Copy link
Contributor

refack commented Feb 16, 2017

Makes it more error prone. Are those commits "sterile"?
I kinda liked this approach and the statement it makes, vendor in GYP as-is and in parallel version track our patches...

@bnoordhuis
Copy link
Member Author

I don't know what you mean by 'sterile.' I also don't understand why you think it's more error prone. Can you elaborate?

@refack
Copy link
Contributor

refack commented Feb 16, 2017

@bnoordhuis "Sterile" as in including only patches tn GYP code, and are they clearly marked.
Error prone, as in "less obvious to find needed steps" and "easier to miss one".

Assuming the procedure for a GYP update now is:

  1. Copy all GYP code (except tests)
  2. Apply patches
  3. Maybe fix some conflicts, and update .patch files
  4. Commit

New process will be:

  1. Same as above
  2. Find all relevant commits that patch GYP. Which are relevant? maybe some are stale? what's the cut off date? Since last GYP Update?
  3. Cherry pick those. Making sure not to miss any.
  4. Maybe resolving conflicts.
  5. Commit.

Both obviously works, IMHO 2nd one more fragile.

@bnoordhuis
Copy link
Member Author

I don't think it's all that different. git log gyp/ shows what patches we float and we have to review them after an upgrade anyway. It's the same process we use in nodejs/node for V8.

All of this may be academic though because upstream gyp is clearly in maintenance mode. I don't expect any big updates before upstream abandons it completely.

@refack
Copy link
Contributor

refack commented Feb 16, 2017

Cool 👍

Copy link
Member

@joaocgreis joaocgreis left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

The patch files are unnecessary, they also exist as commits in the
history.  Delete the files.

PR-URL: #1122
Reviewed-By: João Reis <[email protected]>
@bnoordhuis bnoordhuis merged commit da9cb5f into nodejs:master Mar 20, 2017
@bnoordhuis bnoordhuis deleted the delete-patch-files branch March 20, 2017 14:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants