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

test: ignore the copied entry_point.c #48297

Merged
merged 1 commit into from Jun 6, 2023
Merged

test: ignore the copied entry_point.c #48297

merged 1 commit into from Jun 6, 2023

Conversation

lpinca
Copy link
Member

@lpinca lpinca commented Jun 2, 2023

Add test_cannot_run_js/entry_point.c to
test/js-native-api/.gitignore.

@lpinca
test: ignore the copied entry_point.c
320ec77 
Add `test_cannot_run_js/entry_point.c` to
`test/js-native-api/.gitignore`.
@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests. labels Jun 2, 2023
@lpinca
Copy link
Member Author

lpinca commented Jun 2, 2023

See

node/test/js-native-api/test_cannot_run_js/binding.gyp

Lines 3 to 12 in 66f112d

{
"target_name": "copy_entry_point",
"type": "none",
"copies": [
{
"destination": ".",
"files": [ "../entry_point.c" ]
}
]
},
.

@targos
Copy link
Member

targos commented Jun 2, 2023

Do you know why it's copied instead of being referenced directly like in all other tests?

@lpinca
Copy link
Member Author

lpinca commented Jun 2, 2023

I don't know. The test was created like this.

cc: @gabrielschulhof

@gabrielschulhof
Copy link
Contributor

gabrielschulhof commented Jun 2, 2023 •
edited

@lpinca @targos somehow, having multiple targets in gyp that refer to a path above the root causes test failures. For example, when I tried to write the binding.gyp file as

{
  "target_defaults": {
    "sources": [
        "../entry_point.c",
    ]
  },
  "targets": [
    {
      "target_name": "test_cannot_run_js",
      "sources": [ "test_cannot_run_js.c" ],
      "defines": [ "NAPI_EXPERIMENTAL" ],
    },
    {
      "target_name": "test_pending_exception",
      "sources": [ "test_cannot_run_js.c" ],
      "defines": [ "NAPI_VERSION=8" ],
    }
  ]

I got this failure on https://ci.nodejs.org/job/node-test-commit-osx/52335/nodes=osx11-x64/console and a bunch of other platforms:

08:12:41 rm: ./Release/.deps/Release/obj.target/test_cannot_run_js/../entry_point.o.d.raw: No such file or directory

I tried referring to "../entry_point.c" from each target, and that failed too.

In the end, I had no choice but use this convoluted copy-the-file approach.

@lpinca lpinca added the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2023
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Jun 3, 2023
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

This was referenced Jun 4, 2023
Open
Open
@nodejs-github-bot
Copy link
Collaborator

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

@github-actions github-actions bot mentioned this pull request Jun 6, 2023
Open
20 tasks
@nodejs-github-bot
Copy link
Collaborator

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

@nodejs-github-bot
Copy link
Collaborator

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

@lpinca lpinca added the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 6, 2023
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Jun 6, 2023
@nodejs-github-bot nodejs-github-bot merged commit d15652e into nodejs:main Jun 6, 2023
65 checks passed
@nodejs-github-bot
Copy link
Collaborator

Landed in d15652e

@lpinca lpinca deleted the ignore/entry_point.c branch June 6, 2023 16:23
This was referenced Jun 9, 2023
Open
Open
Open
Open
RafaelGSS pushed a commit that referenced this pull request Jul 3, 2023
@lpinca @RafaelGSS
test: ignore the copied entry_point.c
f1ead59 
Add `test_cannot_run_js/entry_point.c` to
`test/js-native-api/.gitignore`.

PR-URL: #48297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: James M Snell <jasnell@gmail.com>
@RafaelGSS RafaelGSS mentioned this pull request Jul 3, 2023
Merged
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca @Ceres6
test: ignore the copied entry_point.c
1c0af73 
Add `test_cannot_run_js/entry_point.c` to
`test/js-native-api/.gitignore`.

PR-URL: nodejs#48297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: James M Snell <jasnell@gmail.com>
Ceres6 pushed a commit to Ceres6/node that referenced this pull request Aug 14, 2023
@lpinca @Ceres6
test: ignore the copied entry_point.c
03f94c1 
Add `test_cannot_run_js/entry_point.c` to
`test/js-native-api/.gitignore`.

PR-URL: nodejs#48297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: James M Snell <jasnell@gmail.com>
@richardlau richardlau mentioned this pull request Aug 24, 2023
Closed
ruyadorno pushed a commit that referenced this pull request Aug 29, 2023
@lpinca @ruyadorno
test: ignore the copied entry_point.c
f35c4d3 
Add `test_cannot_run_js/entry_point.c` to
`test/js-native-api/.gitignore`.

PR-URL: #48297
Reviewed-By: Tobias Nießen <tniessen@tnie.de>
Reviewed-By: Gabriel Schulhof <gabrielschulhof@gmail.com>
Reviewed-By: Joyee Cheung <joyeec9h3@gmail.com>
Reviewed-By: Darshan Sen <raisinten@gmail.com>
Reviewed-By: LiviaMedeiros <livia@cirno.name>
Reviewed-By: James M Snell <jasnell@gmail.com>
@ruyadorno ruyadorno mentioned this pull request Aug 29, 2023
Merged
@legendecas legendecas mentioned this pull request Sep 6, 2023
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jasnell jasnell jasnell approved these changes

@gabrielschulhof gabrielschulhof gabrielschulhof approved these changes

@tniessen tniessen tniessen approved these changes

@joyeecheung joyeecheung joyeecheung approved these changes

@RaisinTen RaisinTen RaisinTen approved these changes

@LiviaMedeiros LiviaMedeiros LiviaMedeiros approved these changes

Assignees
No one assigned
Labels
needs-ci PRs that need a full CI run. node-api Issues and PRs related to the Node-API. test Issues and PRs related to the tests.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
9 participants
@lpinca @targos @gabrielschulhof @nodejs-github-bot @jasnell @tniessen @joyeecheung @RaisinTen @LiviaMedeiros