Skip to content

test: add postject to fixtures - #45298

Merged
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RaisinTen:test/add-postject-to-fixtures
Dec 15, 2022
Merged

test: add postject to fixtures#45298
nodejs-github-bot merged 1 commit into
nodejs:mainfrom
RaisinTen:test/add-postject-to-fixtures

Conversation

@RaisinTen

@RaisinTenRaisinTen commented Nov 3, 2022

Copy link
Copy Markdown
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-botnodejs-github-bot added needs-ci PRs that need a full CI run. test Issues and PRs related to the tests. labels Nov 3, 2022
@RaisinTenRaisinTen mentioned this pull request Nov 3, 2022
Merged
@RaisinTenRaisinTen added the review wanted PRs that need reviews. label Nov 9, 2022
Trott
Trott approved these changes Nov 23, 2022
View reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Rubber-stamp LGTM.

@Trott

Trott commented Nov 23, 2022

Copy link
Copy Markdown
Member

(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
@RaisinTenRaisinTen 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 reviewed changes
Comment threadtest/fixtures/postject-copy/package.json Outdated

@TrottTrottNov 23, 2022

Copy link
Copy Markdown
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

richardlau commented Nov 23, 2022

Copy link
Copy Markdown
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 reviewed changes

@cjihrigcjihrig left a comment

Copy link
Copy Markdown
Contributor

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

Trott commented Nov 23, 2022

Copy link
Copy Markdown
Member

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

Trott commented Nov 23, 2022

Copy link
Copy Markdown
Member

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 reviewed changes

@TrottTrott left a comment

Copy link
Copy Markdown
Member

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

mhdawson commented Nov 23, 2022

Copy link
Copy Markdown
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-actionsgithub-actionsBot mentioned this pull request Nov 24, 2022
Open
16 tasks
@RaisinTenRaisinTen 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

RaisinTen commented Nov 24, 2022

Copy link
Copy Markdown
MemberAuthor

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
RaisinTenforce-pushed the test/add-postject-to-fixtures branch from 2e38379 to 51a53c8CompareDecember 5, 2022 11:48
@RaisinTen

RaisinTen commented Dec 5, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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 reviewDecember 9, 2022 17:41

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
RaisinTenforce-pushed the test/add-postject-to-fixtures branch from 51a53c8 to 32cd1feCompareDecember 13, 2022 10:53
@RaisinTen

RaisinTen commented Dec 13, 2022•
edited
Loading

Copy link
Copy Markdown
MemberAuthor

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-actionsgithub-actionsBot mentioned this pull request Dec 14, 2022
Open
12 tasks
@nodejs-github-bot

nodejs-github-bot commented Dec 14, 2022

Copy link
Copy Markdown
Collaborator

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

@RaisinTen

RaisinTen commented Dec 14, 2022

Copy link
Copy Markdown
MemberAuthor

Can someone PTAL?

bnb
bnb approved these changes Dec 14, 2022
View reviewed changes

@bnbbnb left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

RSLGTM

@github-actionsgithub-actionsBot mentioned this pull request Dec 15, 2022
Open
9 tasks
@RaisinTenRaisinTen 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-botnodejs-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:mainDec 15, 2022
@nodejs-github-bot

nodejs-github-bot commented Dec 15, 2022

Copy link
Copy Markdown
Collaborator

Landed in 167d7a9

@RaisinTen
RaisinTen deleted the test/add-postject-to-fixtures branch December 15, 2022 05:38
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>
@RafaelGSSRafaelGSS mentioned this pull request Jan 2, 2023
Merged
juanarbol pushed a commit that referenced this pull request Jan 26, 2023
@RaisinTen@juanarbol
test: add postject to fixtures
4aaec07
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>
@juanarboljuanarbol mentioned this pull request Jan 28, 2023
Merged
@RaisinTenRaisinTen added the single-executable Issues and PRs related to single-executable applications label May 5, 2023
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bnbbnbbnb approved these changes
@TrottTrottAwaiting requested review from Trott
@cjihrigcjihrigAwaiting requested review from cjihrig
+2 more reviewers
@aymen94aymen94aymen94 approved these changes
@tony-gotony-gotony-go approved these changes
Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

author readyPRs that have at least one approval, no pending requests for changes, and a CI started.needs-ciPRs that need a full CI run.single-executableIssues and PRs related to single-executable applicationstestIssues and PRs related to the tests.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

9 participants

@RaisinTen@Trott@nodejs-github-bot@richardlau@mhdawson@bnb@cjihrig@aymen94@tony-go