-
Notifications
You must be signed in to change notification settings - Fork 227
Install same emitter only once when generating SDK #5690
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
Changes from all commits
a898146
7bbb41c
38b8f4d
054bd53
4ee0edc
eecb802
9c056d1
fe7d415
e6ccda1
4a09259
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -14,6 +14,87 @@ $ErrorActionPreference = "Stop" | |
| . $PSScriptRoot/common.ps1 | ||
| Install-ModuleIfNotInstalled "powershell-yaml" "0.4.1" | Import-Module | ||
|
|
||
| function GetNpmPackageVersion([string]$packagePath) { | ||
| $packagePath = Resolve-Path $packagePath | ||
| $packageFolder = Split-Path -Parent $packagePath | ||
| $packageName = Split-Path -Leaf $packagePath | ||
| Push-Location $packageFolder | ||
| try { | ||
| $packageVersion = node -p -e "require('./$packageName').version" | ||
| return $packageVersion | ||
| } | ||
| finally { | ||
| Pop-Location | ||
| } | ||
| } | ||
|
|
||
| function GetEmitterDependencyVersion([string]$packagePath) { | ||
| $matchString = "`"$emitterName`": `"(.*?)`"" | ||
| If ((Get-Content -Raw $packagePath) -match $matchString) { | ||
| return $Matches[1] | ||
| } | ||
| } | ||
|
|
||
| function NpmInstallAtRoot() { | ||
| $root = Resolve-Path "$PSScriptRoot/../../.." | ||
| Push-Location $root | ||
| try { | ||
| #default to root/eng/emitter-package.json but you can override by writing | ||
| #Get-${Language}-EmitterPackageJsonPath in your Language-Settings.ps1 | ||
| $replacementPackageJson = "$PSScriptRoot/../../emitter-package.json" | ||
pshao25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if (Test-Path "Function:$GetEmitterPackageJsonPathFn") { | ||
| $replacementPackageJson = &$GetEmitterPackageJsonPathFn | ||
| } | ||
|
|
||
| $replacementVersion = GetEmitterDependencyVersion $replacementPackageJson | ||
| Write-Host "Replacement emitter version: $replacementVersion" | ||
|
|
||
| $installedPath = Join-Path $root "node_modules" $emitterName "package.json" | ||
| Write-Host "Checking installed emitter at $installedPath" | ||
| if (Test-Path $installedPath) { | ||
| $installedVersion = GetNpmPackageVersion $installedPath | ||
| Write-Host "Installed emitter version: $installedVersion" | ||
|
|
||
| if ($installedVersion -eq $replacementVersion) { | ||
| Write-Host "Emitter already installed. Skip installing." | ||
| return | ||
| } | ||
| } | ||
|
|
||
| Write-Host "Installing package at"(Get-Location) | ||
|
|
||
| if (Test-Path "node_modules") { | ||
| Write-Host "Remove existing node_modules at"(Get-Location) | ||
| Remove-Item -Path "node_modules" -Force -Recurse | ||
| } | ||
|
|
||
| $useAlphaNpmRegistry = (Get-Content $replacementPackageJson -Raw).Contains("-alpha.") | ||
| if($useAlphaNpmRegistry) { | ||
| Write-Host "Package.json contains '-alpha.' in the version, Creating .npmrc using public/azure-sdk-for-js-test-autorest feed." | ||
| "registry=https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js-test-autorest/npm/registry/ `n`nalways-auth=true" | Out-File '.npmrc' | ||
| } | ||
|
|
||
| $hasPackageFile = Test-Path "package.json" | ||
|
|
||
| $npmInstallCommand = "npm install --prefix $root $emitterName@$replacementVersion --no-package-lock --omit=dev" | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we start installing as a specific package like this we are making the assumption that there are no other packages in the emitter.package.json file. @raych1 @lmazuel is that assumption valid? If it is perhaps we should avoid any confusion by instead not having package.json file and simply track the package name and version instead.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I see the JS has an additional dependency currently, see https://github.com/Azure/azure-sdk-for-js/blob/main/eng/emitter-package.json. @MaryGao do you know if that extra dependency is needed or will it be included as a dependency of the emitter?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I have no preference between these two. I did this change because @lmazuel mentioned above emitter is a tool not a library.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We should make an explicit decision to make this change as it has larger implications on other things like other dependencies. I don't want to change to this unless we have thought it through and switched to that strategy everywhere.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weshaggard , it's not aligned for all language emitters. I would suggest to use package.json for installation rather than install emitter individually. we could drop package.json to
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I tend to agree we should stick with a package.json file and I believe we can put it under the tempTypeSpec directory it just needs to live under the directory structure of wherever we put the TypeSpec project.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weshaggard , the purpose of @pshao25 is to avoid multiple times installation for emitter. Put package.json under tempTypeSpec directory doesn't work and it should be put under upper parent folder like
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. That is fair we do need to put it in some shared location.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. @weshaggard the design was that we didn't have any additional dependencies in the emitter-package.json. This simulates exactly what a customer might do if they start with a language emitter. They will simply npm install our emitter and that should have ALL necessary dependencies defined in order to run. Since they are required to be defined in the emitters package.json there is no need to duplicate them in the emitter-package.json file.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I don't think this will accommodate the devDependency pinning and emitter-package-lock.json work we're doing. We would prefer to |
||
| Write-Host($npmInstallCommand) | ||
| Invoke-Expression $npmInstallCommand | ||
pshao25 marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| if ($LASTEXITCODE) { exit $LASTEXITCODE } | ||
|
|
||
| if ($hasPackageFile) { | ||
| git restore package.json | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we stick with the
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. No,
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that is the case then that is all the more reason for use to continue to use the |
||
| } | ||
| else { | ||
| rm package.json | ||
| } | ||
|
|
||
| if ($LASTEXITCODE) { exit $LASTEXITCODE } | ||
| } | ||
| finally { | ||
| Pop-Location | ||
| } | ||
| } | ||
|
|
||
| function NpmInstallForProject([string]$workingDirectory) { | ||
| Push-Location $workingDirectory | ||
| try { | ||
|
|
@@ -36,25 +117,7 @@ function NpmInstallForProject([string]$workingDirectory) { | |
| Remove-Item -Path "package-lock.json" -Force | ||
|
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Don't we also want to remove the places we are deleting the files here? I don't think we want to delete those unless we actually do an npm install.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. We need to do that in case user installs emitter under the project folder. If we don't do that, node will not load the emitter installed under root. It will load the emitter installed under the project folder.
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If files like package.json are present under this folder do we think that folks have it committed to the repo? If so then deleting it will cause a change in the files. I think we might actually want to warn people instead of blindly deleting these.
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This place The scenario I'm thinking is: before this change, there will be
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If this is under the temp working directory why would there ever be a package.json and node_modules files under that directory?
Member
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Think about these steps:
Member
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If that is the problem we are worried about we should just have folks remove the entire temp working directory (i.e. |
||
| } | ||
|
|
||
| #default to root/eng/emitter-package.json but you can override by writing | ||
| #Get-${Language}-EmitterPackageJsonPath in your Language-Settings.ps1 | ||
| $replacementPackageJson = Join-Path $PSScriptRoot "../../emitter-package.json" | ||
| if (Test-Path "Function:$GetEmitterPackageJsonPathFn") { | ||
| $replacementPackageJson = &$GetEmitterPackageJsonPathFn | ||
| } | ||
|
|
||
| Write-Host("Copying package.json from $replacementPackageJson") | ||
| Copy-Item -Path $replacementPackageJson -Destination "package.json" -Force | ||
|
|
||
| $useAlphaNpmRegistry = (Get-Content $replacementPackageJson -Raw).Contains("-alpha.") | ||
|
|
||
| if($useAlphaNpmRegistry) { | ||
| Write-Host "Package.json contains '-alpha.' in the version, Creating .npmrc using public/azure-sdk-for-js-test-autorest feed." | ||
| "registry=https://pkgs.dev.azure.com/azure-sdk/public/_packaging/azure-sdk-for-js-test-autorest/npm/registry/ `n`nalways-auth=true" | Out-File '.npmrc' | ||
| } | ||
|
|
||
| npm install --no-lock-file | ||
| if ($LASTEXITCODE) { exit $LASTEXITCODE } | ||
| NpmInstallAtRoot | ||
| } | ||
| finally { | ||
| Pop-Location | ||
|
|
||
Uh oh!
There was an error while loading. Please reload this page.