-
Notifications
You must be signed in to change notification settings - Fork 30.3k
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
build: don't create a new directory for android toolchain #11916
Conversation
@robertchiras You care about the android build, don't you? Can you review? |
I don't know whether it is critical, but the commit message exceeds 50 characters. |
Indeed, doesn't seem critical, but the change looks good to me. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Left some comments. Can you format the commit log according to the style guide?
@robertchiras Thanks for reviewing.
android-configure
Outdated
--install-dir=$TOOLCHAIN \ | ||
--platform=android-21 | ||
if [ -d "$TOOLCHAIN" ]; then | ||
read -r -p "Android toolchain already exists. Do you want to replace it? [y/N] " response |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you wrap this line at 80 columns?
android-configure
Outdated
read -r -p "Android toolchain already exists. Do you want to replace it? [y/N] " response | ||
case "$response" in | ||
[Yy]) | ||
rm -rf $TOOLCHAIN |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The rm -rf
makes me nervous. Can you at least wrap it in double quotes so spaces in the path don't cause bad behavior?
Oh, sorry. Fixed. |
Let make-standalone-toolchain.sh create directory.
FYI: We currently have no way to run this through CI. |
Let make-standalone-toolchain.sh create directory. PR-URL: #11916 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Landed in 757c90e |
Let make-standalone-toolchain.sh create directory. PR-URL: #11916 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
backport? |
ping |
@MylesBorins, sorry, not sure, should I also make PR into other branch for backport? :) |
@TheBeastOfCaerbannog the backport process is covered here, if you could backport to |
Let make-standalone-toolchain.sh create directory. PR-URL: nodejs#11916 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Let make-standalone-toolchain.sh create directory. PR-URL: nodejs/node#11916 Backport-PR-URL: nodejs/node#12975 Reviewed-By: Ben Noordhuis <info@bnoordhuis.nl> Reviewed-By: James M Snell <jasnell@gmail.com>
Starting from commit 90561a... in NDK make-standalone-toolchain.sh requires
--force
option to write toolchain into existing directory and will fail withRefusing to clobber existing install directory
otherwise.But with
--force
option script will fail on older versions of NDK. So best option should be just let make-standalone-toolchain.sh create directory.Checklist
make -j4 test
(UNIX), orvcbuild test
(Windows) passesAffected core subsystem(s)
build