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

build: do not put commands in sources variables #56885

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

zcbenz
Copy link
Contributor

@zcbenz zcbenz commented Feb 3, 2025

The _sources variables are also read by GN and putting anything other than filenames in them would end up with GN errors.

@nodejs-github-bot
Copy link
Collaborator

Review requested:

  • @nodejs/gyp

@nodejs-github-bot nodejs-github-bot added build Issues and PRs related to build files or the CI. needs-ci PRs that need a full CI run. labels Feb 3, 2025
@zcbenz zcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@nodejs-github-bot
Copy link
Collaborator

@zcbenz zcbenz force-pushed the cctest-sources-gn branch from 21bf527 to 8e5fd5a Compare February 3, 2025 06:10
@zcbenz zcbenz force-pushed the cctest-sources-gn branch from 8e5fd5a to 47cb039 Compare February 3, 2025 07:46
@zcbenz zcbenz added the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Feb 3, 2025
@nodejs-github-bot
Copy link
Collaborator

@legendecas
Copy link
Member

legendecas commented Feb 3, 2025

Can we document this rule? Like, add a new gyp-GN inter-op section in https://github.com/nodejs/node/blob/main/doc/contributing/maintaining/maintaining-the-build-files.md.

@zcbenz
Copy link
Contributor Author

zcbenz commented Feb 3, 2025

Didn't know there is a doc on maintaining build files, I will add a section about the GN build in a new pull request, there are more things I want to write down aside from the variables rule.

@nodejs-github-bot
Copy link
Collaborator

@legendecas legendecas added the commit-queue Add this label to land a pull request using GitHub Actions. label Feb 4, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
build Issues and PRs related to build files or the CI. commit-queue Add this label to land a pull request using GitHub Actions. needs-ci PRs that need a full CI run.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants