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

Bump versions for tcgc(0.44.2) and autorest(0.44.1) hotfix #1237

Merged
merged 2 commits into from
Jul 25, 2024

Conversation

tadelesh
Copy link
Member

@tadelesh tadelesh commented Jul 25, 2024

This TCGC hotfix contains two breaking changes:

  1. createSdkContext changed to async function
  • Emitter should use await to get the result
  • The impact is limited to API only, no behavior change.
  1. Built-in types in TCGC has some refactor which removed some kinds of built-in types
  • Refer this to see how to do migration.
  • The impact is limited because if emitter does not care about different kinds of string types, they only need to remove these removed kinds.
  1. Multi-part behavior change
    For multipart, with {@body body: {"prop1": T[]}}, previous tcgc gives "SdkArrayType" whose valueType is "T" for property "prop1"; After this PR merged,
    it is changed to "T" and "prop1" has new optional property "multipartOptions" which has isMulti as True. So language emitters shall judge isMulti to convert "T" to "T[]".
    (it is by design, and see https://github.com/Azure/typespec-azure/pull/987/files)

This version also has:

  1. Support swagger example to be represented in TCGC types, any questions please ping @tadelesh
  2. Support new @multipartBody feature from TypeSpec, any questions please ping @msyyc
  3. Fix of @clientName on sub-clients, cc: @haolingdong-msft
  4. Fix some features used for XML, cc: @jhendrixMSFT

@lirenhe
Copy link
Member

lirenhe commented Jul 25, 2024

When this PR merged, the regen pipeline of Nightly builds will be impacted by 1-2 days waiting all emitters to apply this change.
cc @timotheeguerin , @lmazuel

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

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

This is quite bad that you have to do breaking changes in a patch release and this is affecting the rest of typespec packages. Can this not be done in main

@tadelesh
Copy link
Member Author

tadelesh commented Jul 25, 2024

This is quite bad that you have to do breaking changes in a patch release and this is affecting the rest of typespec packages. Can this not be done in main

@timotheeguerin There are lots of feedback from emitter to not to combine typespec breaking with tcgc breaking which makes emitter's upgrade really hard. So, we decided to do this in release branch.
After the release, I'll do the backmerge to main asap. Is there anything else I do not know that will cause problems?

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

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

Can you check the effect on the spec repo for those changes first as this is bypassing the typespec-next tests. I also I am not at all a fan of the change that was done to autorest emitter as we did a complete opposite change in main. Why is the createTcgcContext async, how much slower is the compilation of autorest right now due to that.

I think you need to start being a little more careful about breaking change when we have a much higher bar for TypeSpec and the autorest emitter is affected by tcgc, you should maybe then prioritize splitting the preprocessor(which you can update whenever you want) from the decorators which should follow the typespec release cycle.

And why is this not done as before just in the main branch but releasing earlier?

@tadelesh
Copy link
Member Author

tadelesh commented Jul 25, 2024

Can you check the effect on the spec repo for those changes first as this is bypassing the typespec-next tests. I also I am not at all a fan of the change that was done to autorest emitter as we did a complete opposite change in main. Why is the createTcgcContext async, how much slower is the compilation of autorest right now due to that.

I think you need to start being a little more careful about breaking change when we have a much higher bar for TypeSpec and the autorest emitter is affected by tcgc, you should maybe then prioritize splitting the preprocessor(which you can update whenever you want) from the decorators which should follow the typespec release cycle.

And why is this not done as before just in the main branch but releasing earlier?

I also prefer the change to sperate create tcgc context and create sdk context in main. Do you think I could check-pick that from main to release? We really do not want the breaking in this PR to be in the same release cycle of TypeSpec.

The reason why we change to use release is we want to separate the thing that we want to release along with typesspec release cycle and the thing we want to have on-demand release. And the perfect solution is what you said, split the decorator thing and sdk thing. @iscai-msft we may need to prioritize it.

@timotheeguerin
Copy link
Member

hhm ok then yeah please cherry pick teh change izzy made first then and update the changelogs to reflect that.
And do test this is not breaking azure rest api spec main branch(you should as well do a Pr there to bump the version after the release)

@tadelesh
Copy link
Member Author

tadelesh commented Jul 25, 2024

hhm ok then yeah please cherry pick teh change izzy made first then and update the changelogs to reflect that. And do test this is not breaking azure rest api spec main branch(you should as well do a Pr there to bump the version after the release)

what is the best way to validate this release does not have breaking for spec main? i tried the artifact from the ci of this pr locally with npm install --force and compile with one arm lib, all works fine. but this could not be work in pr: Azure/azure-rest-api-specs#29965 since there is dep check.

@timotheeguerin
Copy link
Member

You use the artifact from the pr that are changed in the package.json there and then run pwsh ./Eng/scripts/TypespecValidation.ps1 -CheckAll

Or you make a pr with your changes to run the ci(might also need to update the ci script) there is a section in the contributing.md

@tadelesh
Copy link
Member Author

You use the artifact from the pr that are changed in the package.json there and then run pwsh ./Eng/scripts/TypespecValidation.ps1 -CheckAll

Or you make a pr with your changes to run the ci(might also need to update the ci script) there is a section in the contributing.md

thanks for the guide. the validation passed: https://github.com/Azure/azure-rest-api-specs/pull/29965/checks?check_run_id=27918314359. is there any blocker for the release? i'll do the back-merge and update version for spec repo after the release.

Copy link
Member

@timotheeguerin timotheeguerin left a comment

Choose a reason for hiding this comment

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

Thanks @tadelesh, I would recommend we look into splitting this package into:

  • @azure-tools/typespec-client-generator-core
  • @azure-tools/typespec-client-generator-preprocessor

To simplify those processes and issues like that in the future

@timotheeguerin timotheeguerin changed the title [tcgc] release hotfix Bump versions for tcgc(0.44.2) and autorest(0.44.1) hotfix Jul 25, 2024
@tadelesh tadelesh merged commit 7846af9 into release/july-2024 Jul 25, 2024
19 of 21 checks passed
@tadelesh tadelesh deleted the publish/tcgc-hotfix branch July 25, 2024 15:06
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.

6 participants