[workspace] Reorganize patches headed to upstream (#22052)

tools/workspace/README.md

@@ -189,6 +189,24 @@ this will be susceptible to Ubuntu vs macOS differences, so please opt-in to
 the macOS build(s) in Jenkins before merging, using the instructions at
 https://drake.mit.edu/jenkins.html#running-an-on-demand-build.
 
+### Using patch files
+
+When we need to adjust an upstream source release for our purposes, we do that
+with `*.patch` files in Drake, not by forking the repository and pointing to
+the fork.
+
+Patches should live at tools/workspace/foobar/patches/quux.patch. In case the
+change should be upstreamed (even if we haven't filed that pull request yet),
+put the patch in tools/workspace/foobar/patches/upstream/quux.patch to make
+it easier to search for patches that require follow-up action. In case the
+patch should not be upstreamed, you should probably explain why in a comment.
+
+If a change should be upstreamed but our current patch isn't yet groomed to be
+good enough, still place it into `.../patches/upstream/quux.patch`; the idea is
+to separate patches where some further work is required (whether that be opening
+a pull request, or grooming the patch, or even just opening an issue for
+discussion) from patches that are quiescent.
+
 ## Updating pkg_config_repository software versions
 
 Most `pkg_config_repository` calls refer to libraries provided by the host

tools/workspace/abseil_cpp_internal/patches/disable_int128_on_clang.patch

@@ -1,6 +1,9 @@
 Disable __int128 in Clang which triggers linker error under UBSan.
 See https://bugs.llvm.org/show_bug.cgi?id=16404
 
+Reasoning for not upstreaming this patch: would need too much surgery to
+gold-plate it for all of their supported build modes.
+
 --- absl/base/config.h
 +++ absl/base/config.h
 @@ -317,8 +317,7 @@ static_assert(ABSL_INTERNAL_INLINE_NAMESPACE_STR[0] != 'h' ||

tools/workspace/abseil_cpp_internal/patches/hidden_visibility.patch

@@ -2,6 +2,9 @@ Use hidden symbol visibility for Abseil's C++ code.
 
 This probably improves linker and loader throughput.
 
+Reasoning for not upstreaming this patch: would need too much surgery to
+gold-plate it for all of their supported build modes.
+
 --- absl/base/config.h.orig
 +++ absl/base/config.h
 @@ -153,7 +153,7 @@

tools/workspace/abseil_cpp_internal/patches/inline_namespace.patch

@@ -3,6 +3,9 @@ Use an inline namespace for Abseil.
 This prevents Drake's build of Abseil from interfering with a user's
 downstream build of Abseil when the user is using static linking.
 
+Reasoning for not upstreaming this patch: the options header is specifically
+designed for end-user customization, and that's basically all we're doing here.
+
 --- absl/base/options.h
 +++ absl/base/options.h
 @@ -205,8 +205,8 @@

tools/workspace/abseil_cpp_internal/repository.bzl

@@ -9,7 +9,7 @@ def abseil_cpp_internal_repository(
         commit = "03b8d6ea3dc6a0b8c6bcf42503c2053754dab2e4",
         sha256 = "e16fb80e8fad73d5a6af7787c8a52a1b1181a0219133272ade0e200c0a1788d6",  # noqa
         patches = [
-            ":patches/civil_time_linkopts.patch",
+            ":patches/upstream/civil_time_linkopts.patch",
             ":patches/disable_int128_on_clang.patch",
             ":patches/hidden_visibility.patch",
             ":patches/inline_namespace.patch",

tools/workspace/bazelisk/repository.bzl

@@ -23,7 +23,7 @@ def bazelisk_repository(
         sha256 = "4e136f6f1212f28d5c6fdd4cfa3f016d7443831fc98ce8b7ee3caee81ef956fa",  # noqa
         build_file = ":package.BUILD.bazel",
         patches = [
-            ":patches/pull494.patch",
+            ":patches/upstream/pull494.patch",
         ],
         mirrors = mirrors,
     )

tools/workspace/build_bazel_apple_support/patches/no_bazel_features.patch

@@ -6,6 +6,9 @@ compatible with our approach to loading phases.
 
 When we switch to bzlmod, we can reconsider.
 
+Reasoning for not upstreaming this patch: limited value, given the
+pending switch to bzlmod.
+
 --- crosstool/cc_toolchain_config.bzl
 +++ crosstool/cc_toolchain_config.bzl
 @@ -13,7 +13,6 @@

tools/workspace/clang_cindex_python3_internal/patches/clang-15.patch

@@ -1,5 +1,7 @@
 [clang_cindex] Support pre- and post-Clang 15 cursor kinds
 
+Reasoning for not upstreaming this patch: upstream is dead.
+
 --- cindex.py
 +++ cindex.py
 @@ -1313,5 +1313,9 @@

tools/workspace/clarabel_cpp_internal/patches/extern_c.patch

@@ -4,8 +4,8 @@ Drake's vendor_cxx tool doesn't know how to parse `extern "C" { ... }`
 groupings. Work around that by marking all of the individual functions
 one by one.
 
-We should improve vendor_cxx to handle the original file (e.g., rather
-than trying to upstream this patch).
+Reasoning for not upstreaming this patch: it would be more vaulable to
+improve vendor_cxx to handle the original file, instead.
 
 --- include/cpp/DefaultSettings.h
 +++ include/cpp/DefaultSettings.h

tools/workspace/clarabel_cpp_internal/patches/git_submodule.patch

@@ -1,5 +1,8 @@
 [Clarabel.cpp] Pin Clarabel to the Crate (not the git submodule)
 
+Reasoning for not upstreaming this patch: if they want to use submodules,
+then so be it.
+
 --- rust_wrapper/Cargo.toml
 +++ rust_wrapper/Cargo.toml
 @@ -5,4 +5,4 @@

tools/workspace/clarabel_cpp_internal/patches/sdp.patch

@@ -1,5 +1,8 @@
 [Clarabel.cpp] Enable the SDP feature by default
 
+Reasoning for not upstreaming this patch: changing an option, which is
+our prerogative but not relevant for upstream.
+
 --- rust_wrapper/Cargo.toml
 +++ rust_wrapper/Cargo.toml
 @@ -14,6 +14,7 @@ opt-level = 3

tools/workspace/clp_internal/repository.bzl

@@ -10,7 +10,7 @@ def clp_internal_repository(
         sha256 = "0d79ece896cdaa4a3855c37f1c28e6c26285f74d45f635046ca0b6d68a509885",  # noqa
         build_file = ":package.BUILD.bazel",
         patches = [
-            ":patches/missing_include.patch",
+            ":patches/upstream/missing_include.patch",
         ],
         mirrors = mirrors,
     )

tools/workspace/common_robotics_utilities/patches/vendor.patch

@@ -2,6 +2,8 @@
 
 This prevents ODR violations in case downstream code also links to CRU.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- include/common_robotics_utilities/cru_namespace.hpp
 +++ include/common_robotics_utilities/cru_namespace.hpp
 @@ -17,5 +17,5 @@

tools/workspace/conex_internal/patches/no_eigen_io.patch

@@ -2,6 +2,8 @@
 
 Drake builds using EIGEN_NO_IO, so Conex can't use it either.
 
+Reasoning for not upstreaming this patch: Drake-specific build option.
+
 --- conex/debug_macros.h
 +++ conex/debug_macros.h
 @@ -8,7 +8,7 @@

tools/workspace/conex_internal/patches/vendor.patch

@@ -1,5 +1,7 @@
 [conex] Use a custom cc_library rule for vendoring
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- conex/BUILD
 +++ conex/BUILD
 @@ -1,3 +1,8 @@

tools/workspace/conex_internal/repository.bzl

@@ -10,8 +10,8 @@ def conex_internal_repository(
         sha256 = "3f88f45276a1b474946b67e7c650fefd6d7c9dcb48f0c0a11393be3e6adc5ba7",  # noqa
         build_file = ":package.BUILD.bazel",
         patches = [
-            ":patches/debug_macros.patch",
-            ":patches/eigen_namespace_pollution.patch",
+            ":patches/upstream/debug_macros.patch",
+            ":patches/upstream/eigen_namespace_pollution.patch",
             ":patches/no_eigen_io.patch",
             ":patches/vendor.patch",
         ],

tools/workspace/crate_universe/patches/clarabel_blas.patch

@@ -11,6 +11,8 @@ However, since Drake's ultimate "application" is to ship our own
 shared library, we must not allow Rust to govern which libraries to
 link. Thus, we need to delete the lines that perform the linking.
 
+Reasoning for not upstreaming this patch: Drake-specific option.
+
 --- src/algebra/dense/blas/traits.rs
 +++ src/algebra/dense/blas/traits.rs
 @@ -8,8 +8,6 @@ cfg_if::cfg_if! {

tools/workspace/csdp_internal/patches/params_pathname.patch

@@ -1,5 +1,7 @@
 Add option to specify a filename to parse for CSDP parameters.
 
+Reasoning for not upstreaming this patch: upstream is dead.
+
 --- include/declarations.h	2017-07-25 11:44:57.000000000 -0700
 +++ include/declarations.h	2021-06-30 16:17:10.855119121 -0700
 @@ -132,7 +132,7 @@

tools/workspace/csdp_internal/patches/printlevel.patch

@@ -1,5 +1,7 @@
 When no parameters file is in use, do not console print by default.
 
+Reasoning for not upstreaming this patch: Drake-specific default option.
+
 --- lib/initparams.c	2017-07-25 11:44:57.000000000 -0700
 +++ lib/initparams.c	2021-06-30 15:50:10.984899788 -0700
 @@ -35,7 +35,7 @@

tools/workspace/fcl_internal/repository.bzl

@@ -10,7 +10,7 @@ def fcl_internal_repository(
         sha256 = "b055315cad3394b7bb1db8eead56a9063e95ed6cb0f43e12b244ab0bd2ce5be8",  # noqa
         build_file = ":package.BUILD.bazel",
         patches = [
-            ":patches/cassert.patch",
+            ":patches/upstream/cassert.patch",
         ],
         mirrors = mirrors,
     )

tools/workspace/gflags/repository.bzl

@@ -9,7 +9,7 @@ def gflags_repository(
         commit = "v2.2.2",
         sha256 = "34af2f15cf7367513b352bdcd2493ab14ce43692d2dcd9dfc499492966c64dcf",  # noqa
         patches = [
-            ":patches/bazel7.patch",
+            ":patches/upstream/bazel7.patch",
         ],
         mirrors = mirrors,
     )

tools/workspace/googlebenchmark/patches/console_allocs.patch

@@ -1,5 +1,8 @@
 If a memory manager is installed then shows allocs in the summary table.
 
+Reasoning for not upstreaming this patch: don't want to spend the time
+reworking the patch and tests to be suitable.
+
 --- src/console_reporter.cc
 +++ src/console_reporter.cc
 @@ -53,9 +53,11 @@ bool ConsoleReporter::ReportContext(const Context& context) {

tools/workspace/googlebenchmark/patches/remove_overloaded_fixture_set_up.patch

@@ -2,6 +2,8 @@ Avoid GCC warnings about overloaded virtuals
 
 We only need the mutable spelling for Drake code.
 
+Reasoning for not upstreaming this patch: Drake-specific API choice.
+
 --- include/benchmark/benchmark.h
 +++ include/benchmark/benchmark.h
 @@ -1425,12 +1425,8 @@

tools/workspace/googlebenchmark/patches/string_precision.patch

@@ -1,5 +1,8 @@
 Actually respect the requested human-readable precision.
 
+Reasoning for not upstreaming this patch: not sure if the bug is
+observable in their uncustomized build.
+
 --- src/string_util.cc
 +++ src/string_util.cc
 @@ -55,6 +61,8 @@ void ToExponentAndMantissa(double val, double thresh, int precision,

tools/workspace/gtest/patches/add_printers.patch

@@ -8,6 +8,8 @@ The FmtFormatPrinter knows how to ask fmt::to_string to print values.
 We need to inject it prior to the osteam formatter, because we should
 prefer fmt over ostream anytime fmt is offered.
 
+Reasoning for not upstreaming this patch: Drake-specific dependency.
+
 --- googletest/include/gtest/gtest-printers.h
 +++ googletest/include/gtest/gtest-printers.h
 @@ -115,6 +115,9 @@

tools/workspace/highway_internal/patches/linkstatic.patch

@@ -3,8 +3,7 @@
 In general, setting -fvisibility on C++ source code is problematic, but hwy
 upstream has been specifically engineered to allow it.
 
-This situation is unique to Drake, so we do not plan to upstream this
-patch.
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
 
 --- BUILD
 +++ BUILD

tools/workspace/highway_internal/patches/target_get_index_inline_always.patch

@@ -13,7 +13,7 @@ an `#if MSVC` stanza. This is not strictly necessary for Drake, but
 keeps the patch itself compiler-neutral. The second change block is for
 all remaining compilers, and the third change block applies the repair.
 
-These changes should not be upstreamed to highway. The deeper issue is
+Reasoning for not upstreaming this patch: The deeper issue is
 that highway currently relies on delicate handling of deliberate ODR
 violations. Analyzing and fixing ODR issues in highway is beyond the
 scope of work needed for Drake.

tools/workspace/highway_internal/patches/target_update_noinline.patch

@@ -6,6 +6,8 @@ flags it as a violation (non-Drake public symbol in a shared library).
 
 Moving the function to the cc file works around the problem.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- hwy/targets.h
 +++ hwy/targets.h
 @@ -300,11 +300,7 @@

tools/workspace/msgpack_internal/patches/vendor.patch

@@ -4,6 +4,7 @@ For cpp_config_decl.hpp, vendor_cxx gets a bit too greedy and tries to
 span its namespace changes across #ifdef branches. We can avoid that
 outcome by adding a redundant #include statement to help guide it.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
 
 --- include/msgpack/v2/cpp_config_decl.hpp.orig
 +++ include/msgpack/v2/cpp_config_decl.hpp

tools/workspace/mypy_internal/patches/no_retry.patch

@@ -4,6 +4,8 @@ Nothing Drake loads should have nasty side-effects, so there should
 never be any reason to crash and resume. Retrying might slow things
 down, which we definitely do NOT want.
 
+Reasoning for not upstreaming this patch: Drake-specific option value.
+
 --- mypy/moduleinspect.py
 +++ mypy/moduleinspect.py
 @@ -143,5 +143,5 @@

tools/workspace/mypy_internal/patches/timeout.patch

@@ -1,5 +1,7 @@
 Increase timeout from 30 seconds to 360 seconds
 
+Reasoning for not upstreaming this patch: Drake-specific option value.
+
 --- mypy/moduleinspect.py
 +++ mypy/moduleinspect.py
 @@ -157,13 +157,15 @@

tools/workspace/mypy_internal/repository.bzl

@@ -13,8 +13,8 @@ def mypy_internal_repository(
         sha256 = "64bb56c93fbeae322af1bec7105554ac12369687825341cc9b7f0b139b6d688d",  # noqa
         build_file = ":package.BUILD.bazel",
         patches = [
+            ":patches/upstream/reject_double_colon.patch",
             ":patches/no_retry.patch",
-            ":patches/reject_double_colon.patch",
             ":patches/timeout.patch",
         ],
         mirrors = mirrors,

tools/workspace/nanoflann_internal/patches/namespace.patch

@@ -3,6 +3,8 @@ Add a drake_vendor namespace ala tools/workspace/vendor_cxx
 This prevents link-time symbol conflicts in case code downstream of
 Drake wants to use a different build of nanoflann.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- include/nanoflann.hpp.orig
 +++ include/nanoflann.hpp
 @@ -70,6 +70,7 @@

tools/workspace/nlohmann_internal/patches/vendor.patch

@@ -2,6 +2,8 @@ Add vendoring namespace.
 
 This prevents ODR violations in case downstream code also links to json.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- single_include/nlohmann/json.hpp
 +++ single_include/nlohmann/json.hpp
 @@ -131,19 +131,21 @@

tools/workspace/nlopt_internal/patches/gen_enums.patch

@@ -4,6 +4,8 @@ Here we create the same effect by patching the source file directly.
 Separately (in a unit test), we cross-check that this patch and the
 CMake script both produce consistent results.
 
+Reasoning for not upstreaming this patch: Drake-specific build hack.
+
 --- src/api/nlopt-in.hpp.orig
 +++ src/api/nlopt-in.hpp
 @@ -46,6 +46,68 @@

tools/workspace/nlopt_internal/patches/remove_luksan.patch

@@ -4,6 +4,8 @@ That library is licensed under LGPL-2.1+ but the rest of NLopt is
 licensed under MIT or similar notice-only licenses, and we really
 don't want to distribute this code using dynamic linking.
 
+Reasoning for not upstreaming this patch: Drake-specific build option.
+
 --- src/api/optimize.c.orig
 +++ src/api/optimize.c
 @@ -40,7 +40,9 @@

tools/workspace/nlopt_internal/patches/vendor.patch

@@ -3,6 +3,8 @@
 When weaving the inline namespace into this file, we need to ensure that
 it doesn't cross an `#if 0` block.
 
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
+
 --- src/algs/stogo/local.cc
 +++ src/algs/stogo/local.cc
 @@ -11,11 +11,12 @@

tools/workspace/openusd_internal/patches/dlopen_forbidden.patch

@@ -4,6 +4,8 @@ By policy, Drake forbids dlopen (and thus dlclose). We'll change those
 functions in OpenUSD to be no-ops. Any symbol names that we need to
 look up using dlsym should already be loaded by the time we need them.
 
+Reasoning for not upstreaming this patch: Drake-specific build option.
+
 --- pxr/base/arch/library.cpp
 +++ pxr/base/arch/library.cpp
 @@ -40,6 +40,8 @@ DWORD arch_lastLibraryError = 0;

tools/workspace/openusd_internal/patches/namespace.patch

@@ -1,6 +1,6 @@
 [openusd_internal] Adjust top-level namespace
 
-This mitigates Drake build settings, so we don't plan to upstream this patch.
+Reasoning for not upstreaming this patch: Drake-specific vendoring.
 
 --- pxr/pxr.h.in
 +++ pxr/pxr.h.in