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

Does the ChangedMemberOrder validation in smithy diff apply to anything other than C or C++ ? #1861

Closed
mcmasn-amzn opened this issue Jul 17, 2023 · 5 comments · Fixed by #1914
Labels
guidance Question that needs advice or information.

Comments

@mcmasn-amzn
Copy link
Contributor

I have created a smithy diff step in my release system to check for breaking changes. I started getting ChangedMemberOrder errors with this message

union shape members were reordered. This can cause ABI compatibility issues in languages like C and C++ where the layout and alignment of a data structure matters.

Questions

  1. Does this validation apply to anything other than C or C++? I'm most interested in Javascript, Python, Java, and Rust. We don't use C or C++.
  2. Is there a way to suppress specific errors codes from smithy diff? I don't see anything in smithy diff --help which I can use. As long as my languages aren't impacted by ChangedMemberOrder, I'd like to be able to do something like smithy diff --suppress ChangedMemberOrder
@kstich
Copy link
Contributor

kstich commented Jul 17, 2023

created a smithy diff step in my release system

This sounds like a great integration! Is this done through the Smithy CLI in an automated way?

For your questions:

  1. This event type is mostly about code generators built atop language features. Languages that enable dynamic linking usually require a stable ABI to vend updates in a backwards compatible way. Code generators may or may not guarantee stability in this way, so a ChangedMemberOrder event is emitted at the DANGER severity level - essentially saying "make sure you know this is safe for your use case". Which languages support this set of features, and which code generators or toolchains make these guarantees isn't directly Smithy's concern.
  2. This functionality doesn't exist at this time. The --severity flag could be raised to ERROR, but that would cover up other DANGER level events that you may care more about. With smithy diff run in an automated way, we'd recommend that your review framework call out that this doesn't currently apply to your use case and grants an exception -- it's easy to ignore what you don't see.

@mcmasn-amzn
Copy link
Contributor Author

Yes, we have automation that fetches the current 'released' model.json file and compares it to the new version of model.json being built. We're using an Ant build task that invokes the software.amazon.smithy.cli.SmithyCli class.

Also, yes, we can grant ourselves override to our pipeline after reviewing the diffs. I'm not sure myself if ABI stability matters to any of our languages. We don't have dynamically linked binaries, so we ignored it.

@mtdowling
Copy link
Member

Any reason why your members are being reordered?

@mcmasn-amzn
Copy link
Contributor Author

My team had a habit of sorting union members alphabetically. When adding new members, it changes the order. example:

  union Color {
     hsl: Hsl
+    hsv: Hsv
     rgb: Rbg
  }

@jvschneid jvschneid added the guidance Question that needs advice or information. label Jul 24, 2023
mtdowling added a commit that referenced this issue Aug 9, 2023
mtdowling added a commit that referenced this issue Aug 9, 2023
mtdowling added a commit that referenced this issue Aug 10, 2023
@mcmasn-amzn
Copy link
Contributor Author

Thank you for #1914 !

alextwoods pushed a commit to alextwoods/smithy that referenced this issue Sep 15, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
guidance Question that needs advice or information.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants