Skip to content

ci: change pnpm config setting script#673

Merged
Boshen merged 1 commit intomainfrom
ci/pnpm_config
Aug 26, 2025
Merged

ci: change pnpm config setting script#673
Boshen merged 1 commit intomainfrom
ci/pnpm_config

Conversation

@JounQin
Copy link
Contributor

@JounQin JounQin commented Aug 26, 2025

fix #668
close #672

@JounQin JounQin requested review from Boshen and Copilot August 26, 2025 03:48
@graphite-app
Copy link

graphite-app bot commented Aug 26, 2025

How to use the Graphite Merge Queue

Add the label merge to this PR to add it to the merge queue.

You must have a Graphite account in order to use the merge queue. Sign up using this link.

An organization admin has enabled the Graphite Merge Queue in this repository.

Please do not merge from GitHub as this will restart CI on PRs being processed by the merge queue.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR changes the method of configuring pnpm's supported CPU architectures from using pnpm config set command to appending YAML configuration directly to the pnpm-workspace.yaml file.

  • Replaces pnpm config set command with direct YAML file modification
  • Uses echo to append CPU architecture configuration to workspace file

Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.

shell: bash
run: pnpm config set supportedArchitectures.cpu "${CPU_ARCH}"
run: echo "\nsupportedArchitectures:\n cpu:\n - ${CPU_ARCH}" >> pnpm-workspace.yaml
env:
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

The YAML formatting uses literal \n characters instead of actual newlines. This will result in malformed YAML with visible \n text instead of proper line breaks. Use a proper multi-line YAML block or printf instead of echo to ensure correct formatting.

Copilot uses AI. Check for mistakes.
shell: bash
run: pnpm config set supportedArchitectures.cpu "${CPU_ARCH}"
run: echo "\nsupportedArchitectures:\n cpu:\n - ${CPU_ARCH}" >> pnpm-workspace.yaml
env:
Copy link

Copilot AI Aug 26, 2025

Choose a reason for hiding this comment

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

Appending YAML configuration to an existing file without checking if the configuration already exists could result in duplicate entries or conflicts. Consider using a YAML manipulation tool like yq or checking for existing configuration before appending.

Suggested change
env:
# Install yq if not present
if ! command -v yq &> /dev/null; then
YQ_BIN="yq_linux_amd64"
wget -qO $YQ_BIN https://github.com/mikefarah/yq/releases/latest/download/yq_linux_amd64
chmod +x $YQ_BIN
YQ="./$YQ_BIN"
else
YQ="yq"
fi
# Set supportedArchitectures.cpu in pnpm-workspace.yaml
$YQ eval '.supportedArchitectures.cpu = [env(CPU_ARCH)]' -i pnpm-workspace.yaml

Copilot uses AI. Check for mistakes.
@Boshen Boshen merged commit 6e96b11 into main Aug 26, 2025
11 checks passed
@Boshen Boshen deleted the ci/pnpm_config branch August 26, 2025 04:29
@Boshen
Copy link
Member

Boshen commented Aug 26, 2025

@JounQin
Copy link
Contributor Author

JounQin commented Aug 26, 2025

@Boshen Sorry I don't have enough Windows skills to resolve the compatibility issue, how about downgrade pnpm #672 for now?


How about:

node -e 'console.log(`\nsupportedArchitectures:\n  cpu:\n    - ${process.env.CPU_ARCH}`)' >> pnpm-workspace.yaml

- if: ${{ inputs.cpu }}
shell: bash
run: pnpm config set supportedArchitectures.cpu "${CPU_ARCH}"
run: echo "\nsupportedArchitectures:\n cpu:\n - ${CPU_ARCH}" >> pnpm-workspace.yaml
Copy link

@KSXGitHub KSXGitHub Aug 26, 2025

Choose a reason for hiding this comment

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

Windows' bash doesn't intepret \n the same as Linux causes you problem, right?

I would use multiple echos in this case:

      run: |
        echo >> pnpm-workspace.yaml
        echo 'supportedArchitectures:' >> pnpm-workspace.yaml
        echo "  cpu: [ $CPU_ARCH ]" >> pnpm-workspace.yaml

Alternatively, there's echo -n but I don't know how well it would work and I prefer the multiple echo anyway.

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.

ERR_PNPM_CONFIG_SET_DEEP_KEY  Setting deep property path is not supported

4 participants