Skip to content

XCPNG-3989: Commit message list upstream - #7361

Open
contificate wants to merge 6 commits into
xapi-project:masterfrom
contificate:cbarr/master/messages
Open

contificate wants to merge 6 commits into
xapi-project:masterfrom
contificate:cbarr/master/messages

Conversation

@contificate

Copy link
Copy Markdown
Contributor

This is a proof-of-concept of catching various breaking changes to XenAPI. This is quite a niche safety net as it captures 2 cases that aren't captured by the building of Xapi:

  • Permuting parameter order: if you swap the order of parameters (within the required prefix or optional suffix - enforced by another check), this will silently break every client. The corresponding code can remain unchanged because the generated dispatcher invokes the API using labelled arguments (can be applied in any order).
  • Changing a defaulted parameter's value: rare, but this is now detected and must be explicitly noted.

Other larger changes will have to be confronted when you attempt to build the rest of Xapi. However, committing the message list upstream has some idealistic benefits: (1) there's enough information in the generated message listing to produce a typed XenAPI client (this is the approach taken in my xapy project, which uses the documentation's xenapi.json file), (2) by committing messages upstream, it forces the repository state and commits to be obvious about what messages have been changed (along with the possibility, in future, of easily seeing what messages are present between version tags, as they'll be noted upstream).


This work was inspired by 0416e2a (which I came across when generalising optional arguments to permit omission anywhere). This PR subsumes this check and also captures the part that was lacking: new parameters are now detected by comparison against the committed message list (snapshot), so it'll fail the build if you don't promote it (and accept it as a breaking change to the API). Of course, doing such a change requires changing the implementation code, but this makes PR makes it obvious to the developer that they should provide defaults for any new arguments they're appending to extant messages.


As an example of how this works: suppose you permute the ordering of login_with_password(uname, pwd, ...) so that the password comes before the username:

diff --git a/ocaml/idl/datamodel.ml b/ocaml/idl/datamodel.ml
index 861e48852..db3c19c07 100644
--- a/ocaml/idl/datamodel.ml
+++ b/ocaml/idl/datamodel.ml
@@ -39,15 +39,15 @@ module Session = struct
         [
           {
             param_type= String
-          ; param_name= "uname"
-          ; param_doc= "Username for login."
+          ; param_name= "pwd"
+          ; param_doc= "Password for login."
           ; param_release= rio_release
           ; param_default= None
           }
         ; {
             param_type= String
-          ; param_name= "pwd"
-          ; param_doc= "Password for login."
+          ; param_name= "uname"
+          ; param_doc= "Username for login."
           ; param_release= rio_release
           ; param_default= None
           }

Upon trying to build this change, you will be confronting with:

» make                                                                              
[ -z "" ] || (sed -i '/(version.*)/d' dune-project && echo "(version )" >> dune-project)
if test -f /opt/xensource/sm/XE_SR_ERRORCODES.xml; then cp /opt/xensource/sm/XE_SR_ERRORCODES.xml ocaml/sdk-gen/csharp/XE_SR_ERRORCODES.xml; fi
dune build @ocaml/idl/update-dm-lifecycle -j 24 --profile=release --auto-promote || dune build @ocaml/idl/update-dm-lifecycle -j 24 --profile=release --auto-promote
dune exec ocaml/idl/api_compat.exe -j 24 --profile=release -- -check ocaml/idl/messages.ndjson
session.login_with_password: parameter 1 is now pwd: string instead of uname: string

Clients pass arguments by position, these changes to the datamodel
have caused breaking changes to the API.

Add new parameters at the end of the list, with a default. If the breaking change is
intended, update the file with 'dune build @ocaml/idl/update-messages --auto-promote' and note this fact in the commit.
make: *** [Makefile:17: build] Error 1

You can then accept this breaking change (with dune build @ocaml/idl/update-messages --auto-promote ):

dune build @ocaml/idl/update-messages --auto-promote                                               2 ↵
File "ocaml/idl/messages.ndjson", line 1, characters 0-0:
diff --git a/_build/default/ocaml/idl/messages.ndjson b/_build/default/ocaml/idl/messages.ndjson.generated
index 0aabd4754..020af3a2a 100644
--- a/_build/default/ocaml/idl/messages.ndjson
+++ b/_build/default/ocaml/idl/messages.ndjson.generated
@@ -536,7 +536,7 @@
 ["session.create_from_db_file","session ref","session_id: session ref","filename: string"]
 ["session.get_all_subject_identifiers","string set","session_id: session ref"]
 ["session.local_logout","void","session_id: session ref"]
-["session.login_with_password","session ref","uname: string","pwd: string","version: string = \"1.1\"","originator: string = \"\""]
+["session.login_with_password","session ref","pwd: string","uname: string","version: string = \"1.1\"","originator: string = \"\""]
 ["session.logout","void","session_id: session ref"]
 ["session.logout_subject_identifier","void","session_id: session ref","subject_identifier: string"]
 ["session.slave_local_login","session ref","psecret: string"]
Promoting _build/default/ocaml/idl/messages.ndjson.generated to
  ocaml/idl/messages.ndjson.

with this promotion, the check will pass, and you'll be able to build the rest of the toolstack. The other category of change required here is the changing of a parameter's default value. For example, modifying the default originator in login_with_password(uname, pwd, version, originator) to "foo" will error as:

session.login_with_password: parameter 4 is now originator: string = "foo" instead of originator: string = ""

As clients may rely on a default value (unlikely in this exact case), it is a breaking change, so this PR makes that explicit.

Each released message can be emitted with api_compat.exe. The format is
line-delimited JSON (often saved as .ndjson).

You can `dune exec ./ocaml/idl/api_compat.exe -- -print` to see the messages
printed to stdout. The intention is that we'll emit this to a file, committed
upstream, and then add a test that compares the contents for mutations.

Signed-off-by: Colin James <contificate@gmail.com>
Adds a simple parser for the line-delimited JSON format introduced previously.

Signed-off-by: Colin James <contificate@gmail.com>

@last-genius last-genius left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Unlike the existing check, this also covers objects without any params with defaults, and checks for ordering changes. The error messages are helpful and clear - this is a welcome improvement, thank you!

Comment thread ocaml/idl/api_compat.ml Resolved
Comment thread ocaml/idl/messages.ndjson Resolved
Commits the current messages file to the repository.

This was done via redirecting `dune exec ./ocaml/idl/api_compat.exe -- -print`
to ocaml/idl/messages.ndjson.

Signed-off-by: Colin James <contificate@gmail.com>
You can now invoke api_compat.exe with `-check <file>` and it will compare the
datamodel against this file.

We perform a pairwise comparison of message parameters to catch various
potential mutations that may have occurred when modifying the datamodel. E.g.
changing a parameter's default value, a message's return type, permuting the
order of parameters, adding a parameter (at the end) that lacks a default, etc.

The next step is to turn this into a workflow compatible with dune.

Signed-off-by: Colin James <contificate@gmail.com>
Generates a messages.ndjson.generated in the dune build directory, which can
then be compared against what's currently there (for promotion purposes), along
with turning api_compat's -check mode into a test. We also add a promotion rule
alias.

Signed-off-by: Colin James <contificate@gmail.com>
Similar to datamodel_lifecycle.ml, we check the generated messages list against
the one currently committed in the repository. If the check fails, we abandon.
Otherwise, we automatically do promotion (applying the diff to the worktree).

Signed-off-by: Colin James <contificate@gmail.com>
@contificate
contificate force-pushed the cbarr/master/messages branch from f599518 to bf4f95e Compare October 29, 2026 09:57

This branch has not been deployed

No deployments
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.

2 participants