Pin tsp-client under eng/common/tools

[catalinaperalta]

Fixes #11950

[Copilot]

Pull Request Overview

This PR pins the TypeSpec client generator CLI to a specific version to ensure consistent tooling across the project.

• Adds a package.json file to pin @azure-tools/typespec-client-generator-cli to version 0.28.1

Files not reviewed (1)

• eng/common/tools/package-lock.json: Language not supported

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[raych1]

Looks good to me to keep the name as package.json and package-lock.json without mentioning 'tsp-client', since we may add more deps to them in the future.
+@weshaggard @mikeharder to have a look.

[weshaggard]

Do we want any wrappers or at least some instructions on how to consume this? i.e. I assume one would likely npm ci and then npx --no --package @azure-tools/typespec-client-generator-cli from the context of this directory?

As for the directory I'm curious about @mikeharder's take on this to see if we should put it at the root of the eng\common directory or have a npm subdirectory we use or what. I know we talked about having other tools in here but if we are following the cspell approach this would go under a tsp-client folder and we would have a wrapper script that would install and execute it.

[mikeharder]

blocking to leave feedback

[mikeharder]

Do we want any wrappers or at least some instructions on how to consume this? i.e. I assume one would likely npm ci and then npx --no --package @azure-tools/typespec-client-generator-cli from the context of this directory?

As for the directory I'm curious about @mikeharder's take on this to see if we should put it at the root of the eng\common directory or have a npm subdirectory we use or what. I know we talked about having other tools in here but if we are following the cspell approach this would go under a tsp-client folder and we would have a wrapper script that would install and execute it.

High-level, I agree with Wes. I think we should:

1. Copy/paste from eng/common/spelling to eng/common/tsp-client, including the pwsh wrapper.
2. Consider refactoring the pwsh wrapper, to reduce duplicate code between spelling and tsp-client. Maybe a file like eng/common/npm-exec-wrapper.ps1?
3. Update consumers to call eng/common/tsp-client/Invoke-TspClient.ps1
   1. I prefer a simpler name like tsp-client.ps1 (or even tsp-client.[cmd|sh], which can be a tiny wrapper around the pwsh, to give us the flexibility to remove pwsh without a breaking change)

If we ever decide to merge JS tools into a single lockfile, it might look like this:

eng/common/node-tools/package[-lock].json (all node-based tools)
eng/common/spelling/Invoke-CSpell.ps1 (installs and runs under node-tools)
eng/common/tsp-client/Invoke-TspClient.ps1 (installs and runs under node-tools)

Advantages:

• We only need a single JS lockfile in eng/common
• If callers run both tools on a single machine, install perf will be much better

Disadvantages:

• If callers only run a single tool, install perf may be worse, since you're installing unused packages

@weshaggard: Do you have a preference? Since cspell and tsp-client are totally unrelated, and we only have two node-based tools, i'd probably keep them separate for now. But, we can always change this later on, and it won't even be a breaking change, if we don't move or break the PWSH wrappers.

CC: @danieljurek, @benbp

[mikeharder]

see comment

[raych1]

I also prefer to keep spell check and tsp-client as separate since they're totally un-related.

Is it too much to wrap the execution of tsp-client? Are we going to handle all the commands tsp-client provide? Is there any concern to integrate with .NET build target command?

Should we start with minimum version containing tsp-client installation only, then iterate on the improvement?

[weshaggard]

I think having them separate would be good as well.

As for the wrapper it is more of a guide but folks can use it if they like. The .NET command will work but someone still needs to run npm ci before that will succeed. IIRC .NET team did that because they run it a bunch of times in parallel.

[mikeharder]

I also prefer to keep spell check and tsp-client as separate since they're totally un-related.

Fine with me, at least for now. It's a little more work for engsys, to update two lockfiles in eng/common.

Is it too much to wrap the execution of tsp-client? Are we going to handle all the commands tsp-client provide? Is there any concern to integrate with .NET build target command?

The pwsh script should just delegate all CLI arguments to tsp-client. The pwsh script is just responsible for installing the package (if not already installed), then calling npm exec with the same CLI arguments.

For perf, the pwsh script should not run npm ci before every npm exec, since this is too slow. Something like npm i --ignore-scripts --no-audit might be fast enough (should no-op if already up-to-date). Since we control both the package.json and the package-lock.json, and will ensure they stay in sync, in theory it should be sufficient to always use npm i --ignore-scripts --no-audit, even for the first install.

[mikeharder]

As for the wrapper it is more of a guide but folks can use it if they like. The .NET command will work but someone still needs to run npm ci before that will succeed. IIRC .NET team did that because they run it a bunch of times in parallel.

The main complaint I have heard from language repo owners, is they don't want to deal with the nuances of commands like npm. So, I think we should encourage language repos (perhaps except for JS) to use the pwsh wrapper. It ensures they are installing and running the package in a safe, reliable, and consistent way.

[raych1]

The .NET command will work but someone still needs to run npm ci before that will succeed. IIRC .NET team did that because they run it a bunch of times in parallel.

The wrapped powershell script includes the installation part. I expect that invoking the powershell script will work for .NET case.

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[mikeharder]

approved pending fixes to latest comments

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[azure-sdk]

The following pipelines have been queued for testing:
java - template
java - template - tests
js - template
net - template
net - template - tests
python - template
python - template - tests
You can sign off on the approval gate to test the release stage of each pipeline.
See eng/common workflow

[catalinaperalta]

@raych1 is this guidance only for pipelines? I dont think switching to this directory is very user-friendly for regular developers in the language repos.

[raych1]

@catalinaperalta per the best practices NPM recommends, we only suggest user to go to the eng/common/tsp-client folder then run the command from there. Refer to this comment