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

Thirdparty: Harmonize patches to document downstream changes #102242

Merged
merged 1 commit into from
Feb 6, 2025

Conversation

akien-mga
Copy link
Member

@akien-mga akien-mga commented Jan 31, 2025

Due to not having proper documentation on how to do this, various contributors have used slightly different approaches over the years, and we find it's difficult to convey what should be done in a consistent way.

I'll finally get to write such documentation, but first I'm harmonizing the way we do patches so that it can be better documented:

  • Patches should be placed in a patches subfolder, with a descriptive name in kebab case, with .patch extension, starting with a four digit index to know in which order to apply them (e.g. 0001-win7-8-dynamic-load.patch).
  • We no longer include // -- GODOT start -- and // -- GODOT end -- comments around our changes. The patch files should be enough to document what changes we've done.
  • Patches should be the output of git diff, i.e. in Git compatible format and relative to the root of the Git repo. When using the patch command to apply them, that means using patch -p3 < path/to/patch from the thirdparty library's folder, to skip the first 3 folders from the diff paths, e.g. a/thirdparty/embree. Alternatively apply them with git apply from anywhere, or patch -p1 from the root folder of the Godot repository.
  • Thirdparty libraries which have patches should have it identified in thirdparty/README.md in a "Patches:" list of the patch filenames. We identify which PR added the patch in parenthesis, so that context can be found easily without having to write it in the README (which we very rarely did). For example:
## basis_universal

- Upstream: https://github.com/BinomialLLC/basis_universal
- Version: 1.50.0 (051ad6d8a64bb95a79e8601c317055fd1782ad3e, 2024)
- License: Apache 2.0

Files extracted from upstream source:

- `encoder/` and `transcoder/` folders, with the following files removed from `encoder`:
  `jpgd.{cpp,h}`, `3rdparty/{qoi.h,tinydds.h,tinyexr.cpp,tinyexr.h}`
- `LICENSE`

Patches:

- `0001-external-zstd-pr344.patch` (GH-73441)
- `0002-external-jpgd.patch` (GH-88508)
- `0003-external-tinyexr.patch` (GH-97582)
- `0004-remove-tinydds-qoi.patch` (GH-97582)
  • Patches should be generated with git diff > patches/my-patch-name.patch, and the generated patch file added to the same commit that's doing the change to thirdparty code. In other words, the process to generate a patch is:

    1. Copy unmodified thirdparty code, stage it.
    2. Re-applying pre-existing patches in the identified order, stage those changes.
    3. Make required changes to the thirdparty code, minimizing unneeded changes (style, whitespace, etc.).
    4. Generate a patch for those with git diff > patches/000x-descriptive-name.patch, stage both thirdparty code changes and the patch.
    5. Add the name of the patch in thirdparty/README.md where relevant, and with the PR number in parenthesis in the form (GH-xxxxx). If you haven't opened the PR yet, you can make an educated guess based on what's the latest issue/PR number, or amend it in after opening the PR.
    6. If relevant, repeat that workflow for unrelated changes, to try to keep each patch atomic and well described by its file name.
  • When updating a thirdparty library, the process is to remove all existing files (but the patches folder), copy the new files following the thirdparty/README.md instructions, and stage that. Then start reapplying patches in order with git apply patches/0001-some-patch.patch or patch -p3 < patches/0001-some-patch.patch. If any patch fails or notifies about having to do fuzzy hunk matching, stop at that stage and rediff the patch before proceeding to the next step.

@akien-mga akien-mga added this to the 4.x milestone Jan 31, 2025
@akien-mga akien-mga marked this pull request as ready for review January 31, 2025 17:09
@akien-mga akien-mga requested a review from a team as a code owner January 31, 2025 17:09
@akien-mga
Copy link
Member Author

All done, should be ready to review.

fire

This comment was marked as resolved.

@akien-mga

This comment was marked as resolved.

@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from e11ee8e to c44ad4c Compare January 31, 2025 20:22
@akien-mga akien-mga requested review from a team as code owners January 31, 2025 20:22
@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from c44ad4c to c51ae9c Compare January 31, 2025 20:29
@@ -1,5 +1,5 @@
/**************************************************************************/
/* godot.cpp */
/* enet_godot.cpp */
Copy link
Member Author

Choose a reason for hiding this comment

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

FYI @Faless - I decided to rename our ENet Godot socket implementation to enet_godot.{cpp,h} instead of just godot.{cpp,h} to be more explicit.

Comment on lines +667 to +683
#if 0
// WebAssembly supports pthreads, but not pthread_getaffinity_np. Get the number of logical
// threads from the browser or Node.js using JavaScript.
nThreads = MAIN_THREAD_EM_ASM_INT({
const isBrowser = typeof window !== 'undefined';
const isNode = typeof process !== 'undefined' && process.versions != null &&
process.versions.node != null;
if (isBrowser) {
// Return 1 if the browser does not expose hardwareConcurrency.
return window.navigator.hardwareConcurrency || 1;
} else if (isNode) {
return require('os').cpus().length;
} else {
return 1;
}
});
#endif
Copy link
Member Author

Choose a reason for hiding this comment

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

I just restored the original code with #if 0, for documentation's sake and to reduce the risk of conflict.

+ Set<ScanLineEdge> edgeTree;
+ RBSet<ScanLineEdge> edgeTree;
Copy link
Member Author

Choose a reason for hiding this comment

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

This patch was missing changes from Set to RBSet done by Juan a while ago.

@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from c51ae9c to 7cc583b Compare January 31, 2025 20:46
@akien-mga
Copy link
Member Author

akien-mga commented Jan 31, 2025

I reviewed each commit on GitHub and found a few issues in a couple patches, now fixed.

This should finally be ready. Once approved, I'll squash the commits.

Edit: Wait, I spotted some remaining GODOT start comments that need to be cleaned up.

Edit 2: Now it's ready, I'm happy with the current state.

@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from 7cc583b to 7324a22 Compare January 31, 2025 21:26
Copy link
Member

@fire fire left a comment

Choose a reason for hiding this comment

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

My main requirement is that GitHub tests pass. My look-over by hand will be shallow. Approved.

Thanks for getting to this. It has been a problem that's now resolved.

I suggest finding a good place to put the description's instructions for patch creation. This may be enough for a first draft of patch creation documentation for third-party software in Godot Engine.

Edited:

I have created a godot engine documentation bug godotengine/godot-docs#10591

@RandomShaper
Copy link
Member

  • We no longer include // -- GODOT start -- and // -- GODOT end -- comments around our changes. The patch files should be enough to document what changes we've done.

♥️

@akien-mga akien-mga modified the milestones: 4.x, 4.4 Feb 5, 2025
@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from 7324a22 to 8bce6b9 Compare February 5, 2025 22:36
@akien-mga akien-mga requested a review from a team as a code owner February 5, 2025 22:36
@akien-mga
Copy link
Member Author

I squashed the commits, so this should be ready to merge.

It's not exactly critical for 4.4 but it's just a documentation update for the most part, and since it will generate conflicts for cherry-picks, I'd prefer to have 4.4.stable and 4.5-dev share the same baseline so 4.5-dev thirdparty lib updates can easily be cherry-picked to 4.4.x.

@Repiteo
Copy link
Contributor

Repiteo commented Feb 6, 2025

Needs a rebase

Verified

This commit was signed with the committer’s verified signature.
AThousandShips A Thousand Ships
@akien-mga akien-mga force-pushed the thirdparty-patches-cleanup branch from 8bce6b9 to 91907a8 Compare February 6, 2025 00:40
@akien-mga
Copy link
Member Author

Done.

@Repiteo Repiteo merged commit 33dc3bf into godotengine:master Feb 6, 2025
20 checks passed
@Repiteo
Copy link
Contributor

Repiteo commented Feb 6, 2025

Thanks!

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

Successfully merging this pull request may close these issues.

4 participants