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.

Sign up for GitHub

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

Jump to bottom

build: fix make errors that occur in Makefile #55287

Merged
merged 1 commit into from
Oct 14, 2024
Merged

build: fix make errors that occur in Makefile #55287

merged 1 commit into from
Oct 14, 2024

Conversation

kmk324
Copy link
Contributor

@kmk324 kmk324 commented Oct 5, 2024 •
edited
Loading

Fix make errors that occur in Makefile

fix make errors that occur in coverage-clean case and coverage-test case in Makefile
Closes #55288

AS-IS
image

To-BE
image

@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 Oct 5, 2024
@kmk324 kmk324 mentioned this pull request Oct 5, 2024
Closed
@RedYetiDev
Copy link
Member

RedYetiDev commented Oct 5, 2024 •
edited
Loading

Can you explain why this fixes that error? What makes this command better?

FWIW I can't reproduce the issue on my machine.

LiviaMedeiros
LiviaMedeiros approved these changes Oct 6, 2024
View reviewed changes
Copy link
Contributor

@LiviaMedeiros LiviaMedeiros left a comment

Choose a reason for hiding this comment

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

Thanks for catching this. LGTM as is.
The makefile doesn't use -exec anywhere else, so you might want to align it with other finds and use xargs.
Since we don't have guidelines nor linting rules for this and -exec works just fine, feel free to ignore the suggestions.

Makefile Outdated Show resolved Hide resolved
Makefile Outdated Show resolved Hide resolved
@codecov Codecov
Copy link

codecov bot commented Oct 6, 2024 •
edited
Loading

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 87.85%. Comparing base (bbdfeeb) to head (9374cd4).
Report is 57 commits behind head on main.

Additional details and impacted files 
@@            Coverage Diff             @@
##             main   #55287      +/-   ##
==========================================
- Coverage   88.41%   87.85%   -0.57%     
==========================================
  Files         652      652              
  Lines      186612   186762     +150     
  Branches    36062    35778     -284     
==========================================
- Hits       165001   164080     -921     
- Misses      14885    15900    +1015     
- Partials     6726     6782      +56

see 109 files with indirect coverage changes

@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62957/

@LiviaMedeiros LiviaMedeiros added request-ci Add this label to start a Jenkins CI on a PR. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. labels Oct 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@LiviaMedeiros LiviaMedeiros added the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Oct 7, 2024
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/62990/

@kmk324
build: fix make errors that occur in Makefile
9374cd4 
fix make errors that occur in
 coverage-clean case and coverage-test in Makefile
@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63025/

@nodejs-github-bot
Copy link
Collaborator

CI: https://ci.nodejs.org/job/node-test-pull-request/63044/

@jakecastelli jakecastelli added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Oct 13, 2024
@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 14, 2024
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Oct 14, 2024
@nodejs-github-bot nodejs-github-bot merged commit 6d36199 into nodejs:main Oct 14, 2024
58 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in 6d36199

marco-ippolito pushed a commit to marco-ippolito/node that referenced this pull request Oct 14, 2024
@kmk324 @marco-ippolito
build: fix make errors that occur in Makefile
53b5f1e 
fix make errors that occur in
 coverage-clean case and coverage-test in Makefile

PR-URL: nodejs#55287
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: Luigi Pinca <luigipinca@gmail.com>
@kmk324
Copy link
Contributor Author

kmk324 commented Oct 15, 2024

@RedYetiDev
find's -exec option must end with a semicolon,
but in the previous code, depending on the shell, the semicolon was passed to the find command or was passed as the shell's command separator.
With this change, it will work normally in all shell types.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lpinca lpinca lpinca approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. build Issues and PRs related to build files or the CI. commit-queue-squash Add this label to instruct the Commit Queue to squash all the PR commits into the first one. needs-ci PRs that need a full CI run.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

build: make errors that occur in coverage-clean case and coverage-test in Makefile
6 participants
@kmk324 @RedYetiDev @nodejs-github-bot @lpinca @LiviaMedeiros @jakecastelli