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

Don't 'exit' a script meant to be "source"d #35520

Closed
wants to merge 1 commit into from

Conversation

fdgonthier
Copy link
Contributor

@fdgonthier fdgonthier commented Oct 6, 2020

Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.

Fixes: #35519

Checklist
  • make -j4 test (UNIX), or vcbuild test (Windows) passes
  • tests and/or benchmarks are included
  • documentation is changed or added
  • commit message follows commit guidelines

@nodejs-github-bot nodejs-github-bot added build v12.x labels Oct 6, 2020
@aduh95 aduh95 changed the base branch from v12.x to master Oct 19, 2020
@aduh95
Copy link
Contributor

@aduh95 aduh95 commented Oct 19, 2020

@fdgonthier can you please rebase against master?

@MylesBorins MylesBorins removed the v12.x label Nov 3, 2020
Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.
@MylesBorins
Copy link
Member

@MylesBorins MylesBorins commented Nov 3, 2020

I've gone ahead and rebased against master, can anyone confirm this change works? Seems useful for building for android

@aduh95 aduh95 added android review wanted labels Nov 8, 2020
@RaisinTen
Copy link
Member

@RaisinTen RaisinTen commented Nov 15, 2020

Since #35519 is the issue this fixes, I think it would make sense to mention this at the end of the commit message:

Fixes: https://github.com/nodejs/node/issues/35519

targos
targos approved these changes Dec 27, 2020
Copy link
Member

@targos targos left a comment

I see no harm in this change.

@targos targos added request-ci and removed review wanted labels Dec 27, 2020
@github-actions github-actions bot removed the request-ci label Dec 27, 2020
@nodejs-github-bot
Copy link
Contributor

@nodejs-github-bot nodejs-github-bot commented Dec 27, 2020

@aduh95 aduh95 added the commit-queue label Dec 27, 2020
@github-actions github-actions bot removed the commit-queue label Dec 27, 2020
@github-actions
Copy link

@github-actions github-actions bot commented Dec 27, 2020

Commit Queue failed
- Loading data for nodejs/node/pull/35520
✔  Done loading data for nodejs/node/pull/35520
----------------------------------- PR info ------------------------------------
Title      Don't 'exit' a script meant to be "source"d (#35520)
Author     François-Denis Gonthier  (@fdgonthier, first-time contributor)
Branch     fdgonthier:issue-35519 -> nodejs:master
Labels     android, build
Commits    1
 - Don't 'exit' a script meant to be "source"d
Committers 1
 - Myles Borins 
PR-URL: https://github.com/nodejs/node/pull/35520
Fixes: https://github.com/nodejs/node/issues/35519
Reviewed-By: Michaël Zasso 
------------------------------ Generated metadata ------------------------------
PR-URL: https://github.com/nodejs/node/pull/35520
Fixes: https://github.com/nodejs/node/issues/35519
Reviewed-By: Michaël Zasso 
--------------------------------------------------------------------------------
   ✔  Last GitHub Actions successful
   ℹ  Last Full PR CI on 2020-12-27T13:43:24Z: https://ci.nodejs.org/job/node-test-pull-request/35092/
- Querying data for job/node-test-pull-request/35092/
✔  Build data downloaded
   ✔  Last Jenkins CI successful
   ℹ  This PR was created on Tue, 06 Oct 2020 15:32:18 GMT
   ✔  Approvals: 1
   ✔  - Michaël Zasso (@targos) (TSC): https://github.com/nodejs/node/pull/35520#pullrequestreview-558928296
--------------------------------------------------------------------------------
   ✔  No git cherry-pick in progress
   ✔  No git am in progress
   ✔  No git rebase in progress
--------------------------------------------------------------------------------
- Bringing origin/master up to date...
From https://github.com/nodejs/node
 * branch                  master     -> FETCH_HEAD
✔  origin/master is now up-to-date
- Downloading patch for 35520
From https://github.com/nodejs/node
 * branch                  refs/pull/35520/merge -> FETCH_HEAD
✔  Fetched commits as acaa58e60260..fb7894a45d1f
--------------------------------------------------------------------------------
Auto-merging android-configure
warning: inexact rename detection was skipped due to too many files.
warning: you may want to set your merge.renamelimit variable to at least 1469 and retry the command.
[master 137faf9812] Don't 'exit' a script meant to be "source"d
 Author: François-Denis Gonthier 
 Date: Tue Oct 6 11:29:25 2020 -0400
 1 file changed, 3 insertions(+), 3 deletions(-)
   ✔  Patches applied
--------------------------------------------------------------------------------
--------------------------------- New Message ----------------------------------
Don't 'exit' a script meant to be "source"d

Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.

PR-URL: #35520
Fixes: #35519
Reviewed-By: Michaël Zasso targos@protonmail.com

[master cb48481419] Don't 'exit' a script meant to be "source"d
Author: François-Denis Gonthier francois-denis.gonthier@opersys.com
Date: Tue Oct 6 11:29:25 2020 -0400
1 file changed, 3 insertions(+), 3 deletions(-)
✖ cb4848141966adb8ba66394ebc8fdd2aaa99c6fe
✔ 6:7 Valid fixes URL. fixes-url
✔ 0:0 blank line after title line-after-title
✔ 0:0 line-lengths are valid line-length
✔ 0:0 metadata is at end of message metadata-end
✔ 5:8 PR-URL is valid. pr-url
✔ 0:0 reviewers are valid reviewers
✖ 0:0 Missing subsystem. subsystem
✔ 0:0 Title is formatted correctly. title-format
✔ 0:0 Title is <= 50 columns. title-length

ℹ Please fix the commit message and try again.

https://github.com/nodejs/node/actions/runs/447274178

@github-actions github-actions bot added the commit-queue-failed label Dec 27, 2020
@targos targos self-assigned this Dec 27, 2020
targos added a commit that referenced this issue Dec 27, 2020
Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.
Fix the "android-configure" script to use "return" instead of "exit".

PR-URL: #35520
Fixes: #35519
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@targos
Copy link
Member

@targos targos commented Dec 27, 2020

Landed in 6c50d74

@targos targos closed this Dec 27, 2020
@targos targos removed the commit-queue-failed label Dec 27, 2020
@targos targos removed their assignment Dec 27, 2020
danielleadams added a commit that referenced this issue Jan 12, 2021
Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.
Fix the "android-configure" script to use "return" instead of "exit".

PR-URL: #35520
Fixes: #35519
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos added a commit that referenced this issue May 1, 2021
Running exit in a script meant to be sourced means the user shell will
exit, which prevents seeing the error message, and is generally very
annoying.
Fix the "android-configure" script to use "return" instead of "exit".

PR-URL: #35520
Fixes: #35519
Reviewed-By: Michaël Zasso <targos@protonmail.com>
@danielleadams danielleadams mentioned this pull request May 3, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
android build
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

6 participants
X Tutup