-
Notifications
You must be signed in to change notification settings - Fork 1.4k
chore(build) Docker fixes #1542
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
base: master
Are you sure you want to change the base?
Conversation
| if (error?.exitCode) { | ||
| console.error(chalk.red(`Exit code: ${error.exitCode} ${error.constructor?.name}`)); | ||
| } | ||
| console.error(chalk.red(String(error))); |
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.
I was getting back a Now fixed in ChildProcess object in the error variable, which is not really desirable, but that's what happened. So I reported it by looking for the exitCode property.spawn_stream.mjs
Instead of logging error.message I log String(error) which should produce almost identical output for errors (it tends to have an extra "Error: " on the front). But in the undesirable case where a non-error is caught, this will at least print something.
I needed this extra error handling because, for some reason cordova/setup action was throwing a ChildProcess with exit code 1.
After debugging it here, I fixed spawnStream() which was throwing an object instead of an error, and also added a process.exit(0) to stop the failure itself.
| } | ||
|
|
||
| // Without this, the script exits with code 1, causing the caller to fail | ||
| process.exit(0); |
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.
I have no idea why this was needed, but it was!
This (silent) failure was why I added the extra error handling run run_action.mjs.
| const uid = os.userInfo().uid; | ||
| if (uid !== 0) { | ||
| console.error(`It might be non-writable because you are running as ${uid} instead of running as root.`); | ||
| console.error(`A workaround is to run 'node ./src/cordova/build.action.mjs' instead of 'npm run action cordova/build'`); |
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.
For some reason ./tools/build.sh npm run action cordova/build drop to a user account inside the container,
while ./tools/build.sh node ./src/cordova/build.action.mjs runs as root.
If we can make the first one run as root, then this commit could probably be removed/reverted.
| COPY ./setup_linux_android.sh ./setup_linux_android.sh | ||
|
|
||
| # We define the variable here, so it can be seen by both the setup script, and the running container | ||
| ENV ANDROID_SDK_ROOT=${ANDROID_SDK_ROOT:-"/opt/android-sdk"} |
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.
Without this, when we finally run the build, it doesn't know where to find the SDK.
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.
These are some problems I had, and how I resolved them.
Up to you whether you want to merge this, or address the issues elsewhere in code or docs.
I'm hoping this will save you some time, rather than cost more.
Feel free to close if you are planning on other solutions. I will still have my branch when I need it!
…y type of error object into String
caf0227 to
047ff83
Compare
6c2a801 to
724fe24
Compare
…'t need special handling
| resolve(childProcess); | ||
| } else { | ||
| reject(childProcess); | ||
| reject(new Error(`Process '${command}' failed with error code ${code}`)); |
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.
As with throw, we should always reject with an Error object.
I have managed to build with Docker but it took a few fixes. I'm on Linux.
I'll share them with you. If you run into problems with Docker builds, these may help you!
May be of interest to @fortuna or @daniellacosse who have been working around this area.