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

test: add postject to fixtures #45298

Merged

Conversation

RaisinTen
Copy link
Contributor

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 added the review wanted PRs that need reviews. label Nov 9, 2022
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.

@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
},
"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.

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
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.

@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
Contributor 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.

@RaisinTen
Copy link
Contributor Author

RaisinTen commented Dec 5, 2022

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

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

license has been added to tool

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
Copy link
Contributor Author

RaisinTen commented Dec 13, 2022

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.

@nodejs-github-bot
Copy link
Collaborator

@RaisinTen
Copy link
Contributor Author

Can someone PTAL?

Copy link
Contributor

@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

@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
@nodejs-github-bot
Copy link
Collaborator

Landed in 167d7a9

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

Successfully merging this pull request may close these issues.

None yet

9 participants