Add labels to PRs with potential ABI breaks #15682
Conversation
|
I generated the list of public headers by applying the following patch win32/build/confutils.js | 18 ++++++++++++++++++
1 file changed, 18 insertions(+)
diff --git a/win32/build/confutils.js b/win32/build/confutils.js
index 95a4e5ce3c..796fb0d167 100644
--- a/win32/build/confutils.js
+++ b/win32/build/confutils.js
@@ -67,6 +67,8 @@ var sapi_enabled = new Array();
/* Store the headers to install */
var headers_install = new Array();
+var headers_install_yaml = new Array();
+
/* Store unknown configure options */
var INVALID_CONFIG_ARGS = new Array();
@@ -2090,6 +2092,7 @@ function generate_files()
STDOUT.WriteLine("Generating files...");
generate_tmp_php_ini();
generate_makefile();
+ generate_headers_install_yaml();
if (!MODE_PHPIZE) {
generate_internal_functions();
generate_config_h();
@@ -2630,6 +2633,17 @@ function generate_makefile()
MF.Close();
}
+function generate_headers_install_yaml()
+{
+ STDOUT.WriteLine("Generating Makefile");
+ var yaml = FSO.CreateTextFile("public_headers.yaml", true);
+ headers_install_yaml = headers_install_yaml.sort();
+ for (var i in headers_install_yaml) {
+ yaml.WriteLine(" - '" + headers_install_yaml[i] + "'");
+ }
+ yaml.Close();
+}
+
function ADD_FLAG(name, flags, target)
{
if (target != null) {
@@ -2863,11 +2877,13 @@ function PHP_INSTALL_HEADERS(dir, headers_list)
src += '\\';
}
headers_install[headers_install.length] = [dir + src, 'dir',''];
+ headers_install_yaml[headers_install_yaml.length] = (dir + src).replace(new RegExp("\\\\", "g"), "/") + "*.h";
ADD_FLAG("INSTALL_HEADERS_DIR", dir + src);
found = true;
} else if (isfile) {
dirname = FSO.GetParentFolderName(dir + src);
headers_install[headers_install.length] = [dir + src, 'file', dirname];
+ headers_install_yaml[headers_install_yaml.length] = (dir + src).replace(new RegExp("\\\\", "g"), "/");
ADD_FLAG("INSTALL_HEADERS", dir + src);
found = true;
} else {
@@ -2879,10 +2895,12 @@ function PHP_INSTALL_HEADERS(dir, headers_list)
src += '\\';
}
headers_install[headers_install.length] = [path, 'dir',''];
+ ERROR("unsupported for YAML");
ADD_FLAG("INSTALL_HEADERS_DIR", path);
} else if (isfile) {
dirname = FSO.GetParentFolderName(path);
headers_install[headers_install.length] = [path, 'file', dir];
+ ERROR("unsupported for YAML");
ADD_FLAG("INSTALL_HEADERS", dir + src);
found = true;
}and then ran |
|
I don't know if I screwed up somewhere, or if it is not possible to test the labelling in this PR right away. Maybe @iluuu1994 knows? |
|
The output says:
This is because we're not actually checking out the repo when running the labeler: https://github.com/php/php-src/blob/master/.github/workflows/labeler.yml I suspect that the file is fetched from the default branch, but I don't know for sure. |
ABI breaks are not supposed to happen after feature freeze, i.e. when the PHP API numbers have been bumped. To make it easier to notice inadvertent ABI breaks, we automatically add an "ABI break" label to all PRs which modify public (aka. installed) header files. Some of these modifications do not constitute an ABI break (e.g. adding a comment to a header file), but we rely on natural intelligence to sort that out. That means these labels should be removed manually, if they are not appropriate, but if they are, the PR should not be merged into any stable branch. For the master branch, where ABI breaks are permissible, the labels should still be removed if irrelevant, but kept when the PR is merged. Since tests are futile[1], we leave that to further (test) PRs. [1] <php#15682 (comment)>
cmb69
changed the title
|
Actually, I don't think it makes sense to rebase onto PHP-8.2, since running CI is useless anyway, and there are different sets of public headers for each minor PHP version. PHP-8.2PHP-8.3So this can be applied either way. I'm not quite sure if we should commit the patch above; that would make it easy to recreate the list of public headers. Or maybe someone has a better (or at least more portable solution). |
Actually, that might be a good thing in this case, since there have been changes about which headers are installed between the minor PHP versions. |
|
I might be missing something, but #15988 was tagged as an ABI break and it only changed |
|
Indeed, good catch. Sadly, the job output is not helpful. https://github.com/php/php-src/actions/runs/10983460534/job/30492852826 |
Nope, see #15988 (comment). @iluuu1994, I have no idea what's going on there, PR #15975 was also not supposed to be labeled (although it makes sense; an omission that should be fixed). It seems that PRs targeting |
|
@cmb69 In that case, you could try to add a |
Ah, right, that should be done anyway, since the list of published headers is different for each branch. However, this shouldn't matter for the PRs where the labeler went wrong. I wonder whether the problem is because were still using v4, but v5 only on master. |
Yep. I've just went with the syntax for |
|
Small sanity check successful: PR #16008. \o/ |
ABI breaks are not supposed to happen after feature freeze, i.e. when
the PHP API numbers have been bumped. To make it easier to notice
inadvertent ABI breaks, we automatically add an "ABI break" label to
all PRs which modify public (aka. installed) header files. Some of
these modifications do not constitute an ABI break (e.g. adding a
comment to a header file), but we rely on natural intelligence to sort
that out. That means these labels should be removed manually, if they
are not appropriate, but if they are, the PR should not be merged into
any stable branch. For the master branch, where ABI breaks are
permissible, the labels should still be removed if irrelevant, but kept
when the PR is merged.
TODO:
"Potential ABI break"?"ABI break")[ ] Target PHP-8.2(see below)