Fix arch amd64 now x86_64 - #2
Conversation
Bumps [semver](https://github.com/npm/node-semver) from 7.6.3 to 7.7.3. - [Release notes](https://github.com/npm/node-semver/releases) - [Changelog](https://github.com/npm/node-semver/blob/main/CHANGELOG.md) - [Commits](npm/node-semver@v7.6.3...v7.7.3) --- updated-dependencies: - dependency-name: semver dependency-version: 7.7.3 dependency-type: direct:production update-type: version-update:semver-minor ... Signed-off-by: dependabot[bot] <support@github.com>
…n/semver-7.7.3 build(deps): bump semver from 7.6.3 to 7.7.3
…rch> linux and macos releases are using .tar.gz files instead of .zip Signed-off-by: Eduardo B. Saporski <esaporski@protonmail.com>
This will not work for past releases where amd64 was correct. Signed-off-by: Oliver Ford <dev@ojford.com>
|
(now signed-off! cf. tofuutils#17) |
There was a problem hiding this comment.
Pull request overview
This pull request updates the architecture naming convention from "amd64" to "x86_64" in the build file name construction. The PR description notes this change is necessary for newer releases, though it will not work for past releases where "amd64" was correct.
Changes:
- Added architecture mapping logic to convert "amd64" to "x86_64" in the
getBuildmethod - Applied the change in both source code and distribution bundle
Reviewed changes
Copilot reviewed 1 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
| lib/releases.js | Added conditional mapping to convert "amd64" arch parameter to "x86_64" before constructing the build file name |
| dist/index.js | Applied the same architecture mapping in the compiled distribution bundle |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if (arch === 'amd64') arch = 'x86_64'; | ||
|
|
||
| const requiredName = `tenv_v${this.version}_${platform}_${arch}.${fileExtension}`; |
There was a problem hiding this comment.
Mutating the arch parameter directly is problematic. This modifies the caller's variable if it's passed by reference or could cause confusion about the parameter's purpose. Consider creating a new variable instead, for example: const mappedArch = arch === 'amd64' ? 'x86_64' : arch; and then use mappedArch in the requiredName construction.
| if (arch === 'amd64') arch = 'x86_64'; | |
| const requiredName = `tenv_v${this.version}_${platform}_${arch}.${fileExtension}`; | |
| const mappedArch = arch === 'amd64' ? 'x86_64' : arch; | |
| const requiredName = `tenv_v${this.version}_${platform}_${mappedArch}.${fileExtension}`; |
| getBuild (platform, arch) { | ||
| let fileExtension = 'tar.gz'; | ||
| if (os.platform().startsWith('win')) fileExtension = 'zip'; | ||
| if (arch === 'amd64') arch = 'x86_64'; |
There was a problem hiding this comment.
The new architecture mapping logic from 'amd64' to 'x86_64' lacks test coverage. Since the repository has existing tests for this module (lib/test/releases.test.js), consider adding test cases for the getBuild method that verify the architecture mapping works correctly, particularly testing with both 'amd64' input (expecting it to map to 'x86_64' in the build name) and 'x86_64' input (expecting it to remain unchanged).
This will not work for past releases where amd64 was correct.