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 Issues and PRs related to build files or the CI. v12.x labels Oct 6, 2020
@aduh95 aduh95 changed the base branch from v12.x to master October 19, 2020 20:37
@aduh95
Copy link
Contributor

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
Contributor

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

@aduh95 aduh95 added android Issues and PRs related to the android platform. review wanted PRs that need reviews. labels Nov 8, 2020
@RaisinTen
Copy link
Contributor

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

Copy link
Member

@targos targos left a comment

Choose a reason for hiding this comment

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

I see no harm in this change.

@targos targos added request-ci Add this label to start a Jenkins CI on a PR. and removed review wanted PRs that need reviews. labels Dec 27, 2020
@github-actions github-actions bot removed the request-ci Add this label to start a Jenkins CI on a PR. label Dec 27, 2020
@nodejs-github-bot
Copy link
Collaborator

@aduh95 aduh95 added the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 27, 2020
@github-actions github-actions bot removed the commit-queue Add this label to land a pull request using GitHub Actions. label Dec 27, 2020
@github-actions
Copy link
Contributor

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 [email protected]

[master cb48481419] Don't 'exit' a script meant to be "source"d
Author: François-Denis Gonthier [email protected]
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 An error occurred while landing this pull request using GitHub Actions. label Dec 27, 2020
@targos targos self-assigned this Dec 27, 2020
targos pushed a commit that referenced this pull request 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 <[email protected]>
@targos
Copy link
Member

targos commented Dec 27, 2020

Landed in 6c50d74

@targos targos closed this Dec 27, 2020
@targos targos removed the commit-queue-failed An error occurred while landing this pull request using GitHub Actions. label Dec 27, 2020
@targos targos removed their assignment Dec 27, 2020
danielleadams pushed a commit that referenced this pull request 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 <[email protected]>
@danielleadams danielleadams mentioned this pull request Jan 12, 2021
targos pushed a commit that referenced this pull request 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 <[email protected]>
@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 Issues and PRs related to the android platform. build Issues and PRs related to build files or the CI.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Don't "exit" android-configure, which is meant to be "source"d
6 participants