X Tutup
The Wayback Machine - https://web.archive.org/web/20230114210535/https://github.com/nodejs/node/pull/45298
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

Jump to bottom

test: add postject to fixtures #45298

Merged
merged 1 commit into from Dec 15, 2022
Merged

test: add postject to fixtures #45298

merged 1 commit into from Dec 15, 2022

Conversation

RaisinTen
Copy link
Member

This should make it possible to test out the creation of Single Executable Applications on a PR without making outbound requests to download and run postject using npm.

This is needed for #45038.

Refs: https://github.com/nodejs/single-executable/blob/1840f3d9c5f4fa0d29aabd5618c4ff9745f7be87/meetings/2022-10-31.md?plain=1#L75-L76
Signed-off-by: Darshan Sen raisinten@gmail.com

cc @nodejs/single-executable

@nodejs-github-bot nodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 3, 2022
@RaisinTen RaisinTen mentioned this pull request Nov 3, 2022
Open
@RaisinTen RaisinTen added the review wanted PRs that need reviews. label Nov 9, 2022
Trott
Trott approved these changes Nov 23, 2022
View changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

Rubber-stamp LGTM.

@Trott
Copy link
Member

Trott commented Nov 23, 2022

(It does feel a bit strange adding something to test/fixtures without also adding something somewhere else that uses it, but I'm not going to worry about that if no one else is. I assume the plan is to use it in subsequent PRs.)

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@RaisinTen RaisinTen added the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 23, 2022
Trott
Trott reviewed Nov 23, 2022
View changes
test/fixtures/postject-copy/package.json
},
"keywords": [],
"author": "",
"license": "ISC",
Copy link
Member

Choose a reason for hiding this comment

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

Optional nit: If this is code we're authoring and not copying from somewhere else, perhaps we should use the license that we use for the entire code base instead?

Suggested change
"license": "ISC",
"license": "MIT",

@richardlau
Copy link
Member

This still has similar concerns to #45066 -- this is adding a 4mb binary WASM blob.

cjihrig
cjihrig approved these changes Nov 23, 2022
View changes
Copy link
Contributor

@cjihrig cjihrig left a comment

Choose a reason for hiding this comment

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

LGTM with the suggested license change.

@Trott
Copy link
Member

Trott commented Nov 23, 2022

Do we need to update tools/license-builder.sh? We list things like gtest and ESLint that we include as part of the code base, so I think the answer is "yes"?

@Trott
Copy link
Member

Trott commented Nov 23, 2022

This still has similar concerns to #45066 -- this is adding a 4mb binary WASM blob.

Is this a blocker or merely a raised eyebrow?

Trott
Trott previously requested changes Nov 23, 2022
View changes
Copy link
Member

@Trott Trott left a comment

Choose a reason for hiding this comment

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

I'm pretty sure we need to add postject to tools/license-builder.sh. Other than that, though, seems OK to me.

@mhdawson
Copy link
Member

In order to address this concern

This still has similar concerns to #45066 -- this is adding a 4mb binary WASM blob.

Could we intergrate similar to https://github.com/nodejs/node/tree/main/tools/lint-md where what gets added is any additional wrappers needed by the test as well as a package.json which as part of the make test would be installed ?

I think https://github.com/nodejs/node/tree/main/tools/clang-format would be another example of a tool that we do something similar for.

@github-actions github-actions bot mentioned this pull request Nov 24, 2022
Open
16 tasks
@RaisinTen RaisinTen removed the author ready PRs that have at least one approval, no pending requests for changes, and a CI started. label Nov 24, 2022
@RaisinTen
Copy link
Member Author

FWIW, if the concern is about the size of the thing being added, we already include eslint - https://github.com/nodejs/node/tree/main/tools/node_modules/eslint which is 21MB and postject is smaller than that.

Regarding this being a blob, which is not something Linux distros like because it's unreadable - it's not something that should affect them because this will not get built as a part of the node binary.

So I'm not sure how this can be a problem.

This was referenced Nov 25, 2022
Open
Open
Open
Open
Open
@RaisinTen RaisinTen force-pushed the test/add-postject-to-fixtures branch from 2e38379 to 51a53c8 Compare Dec 5, 2022
@RaisinTen
Copy link
Member Author

RaisinTen commented Dec 5, 2022 •
edited

I'm pretty sure we need to add postject to tools/license-builder.sh. Other than that, though, seems OK to me.

@Trott done, PTAL

EDIT: Oh, nvm. Looks like tools/license-builder.sh failed, that's why the LICENSE file wasn't updated here. It's failing because downloading files from the raw github links like

node/tools/license-builder.sh

Line 72 in 1287530

licenseText="$(curl -sL https://raw.githubusercontent.com/bestiejs/punycode.js/HEAD/LICENSE-MIT.txt)"
seems to be timing out for some reason. 🤔

@Trott Trott dismissed their stale review Dec 9, 2022

license has been added to tool

@RaisinTen
test: add postject to fixtures
32cd1fe 
This should make it possible to test out the creation of Single
Executable Applications on a PR without making outbound requests to
download and run postject using npm.

This is needed for nodejs#45038.

Refs: https://github.com/nodejs/single-executable/blob/1840f3d9c5f4fa0d29aabd5618c4ff9745f7be87/meetings/2022-10-31.md?plain=1#L75-L76
Signed-off-by: Darshan Sen <raisinten@gmail.com>
@RaisinTen RaisinTen force-pushed the test/add-postject-to-fixtures branch from 51a53c8 to 32cd1fe Compare Dec 13, 2022
@RaisinTen
Copy link
Member Author

RaisinTen commented Dec 13, 2022 •
edited

Turns out it was a connection issue from my end. I switched to a different network to run the script, so now the license changes should be okay. PTAL.

@nodejs-github-bot

This comment was marked as outdated.

Sign in to view
@github-actions github-actions bot mentioned this pull request Dec 14, 2022
Open
12 tasks
@nodejs-github-bot
Copy link
Contributor

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

@RaisinTen
Copy link
Member Author

Can someone PTAL?

@RaisinTen RaisinTen requested review from Trott and cjihrig Dec 14, 2022
bnb
bnb approved these changes Dec 14, 2022
View changes
Copy link
Member

@bnb bnb left a comment

Choose a reason for hiding this comment

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

RSLGTM

@github-actions github-actions bot mentioned this pull request Dec 15, 2022
Open
9 tasks
@RaisinTen RaisinTen added author ready PRs that have at least one approval, no pending requests for changes, and a CI started. commit-queue Add this label to land a pull request using GitHub Actions. and removed review wanted PRs that need reviews. labels Dec 15, 2022
@nodejs-github-bot nodejs-github-bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 15, 2022
@nodejs-github-bot nodejs-github-bot merged commit 167d7a9 into nodejs:main Dec 15, 2022
50 checks passed
@nodejs-github-bot
Copy link
Contributor

Landed in 167d7a9

@RaisinTen RaisinTen deleted the test/add-postject-to-fixtures branch Dec 15, 2022
This was referenced Dec 16, 2022
Open
Open
targos pushed a commit that referenced this pull request Jan 1, 2023
@RaisinTen @targos
test: add postject to fixtures
5de08ef 
This should make it possible to test out the creation of Single
Executable Applications on a PR without making outbound requests to
download and run postject using npm.

This is needed for #45038.

Refs: https://github.com/nodejs/single-executable/blob/1840f3d9c5f4fa0d29aabd5618c4ff9745f7be87/meetings/2022-10-31.md?plain=1#L75-L76
Signed-off-by: Darshan Sen <raisinten@gmail.com>
PR-URL: #45298
Reviewed-By: Colin Ihrig <cjihrig@gmail.com>
Reviewed-By: Tierney Cyren <hello@bnb.im>
@RafaelGSS RafaelGSS mentioned this pull request Jan 2, 2023
Merged
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@bnb bnb

@aymen94 aymen94

@tony-go tony-go

@Trott Trott

@cjihrig cjihrig

Assignees
No one assigned
Labels
author ready PRs that have at least one approval, no pending requests for changes, and a CI started. needs-ci PRs that need a full CI run. 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
@RaisinTen @Trott @nodejs-github-bot @richardlau @mhdawson @bnb @cjihrig @aymen94 @tony-go
X Tutup