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

Add linux-arm64, android-arm and android-arm64 prebuilds #587

Merged
merged 25 commits into from
Feb 1, 2019

Conversation

vweevers
Copy link
Member

@vweevers vweevers commented Jan 27, 2019

Closes #574, closes #360

package.json Outdated
@@ -14,7 +14,11 @@
"hallmark": "hallmark --fix",
"dependency-check": "dependency-check . test/*.js bench/*.js",
"prepublishOnly": "npm run dependency-check",
"prebuildify-cross-armv7": "prebuildify-cross --platform=linux --arch=arm --arm-version=7 -- -t 8.14.0 --napi --strip"
"prebuild-arm": "npm run prebuild-linux-arm && npm run prebuild-linux-arm64 && npm run prebuild-android-arm && npm run prebuild-android-arm64",
Copy link
Member Author

Choose a reason for hiding this comment

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

Should we shorten this with e.g. npm-run-all?

Copy link
Member

Choose a reason for hiding this comment

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

Yes, good idea. (TIL about it)

package.json Outdated Show resolved Hide resolved
@vweevers
Copy link
Member Author

vweevers commented Jan 27, 2019

prebuildify-cross fails on prebuildify-cross --platform=linux --arch=arm64 due to invalid args. I think it needs --arm-version 8 (and that's a gap in the documentation)?

It hits this code branch: https://github.com/ahdinosaur/prebuildify-cross/blob/c4748f7fc5ad3e0e9bbadc6de2304528cf33406a/build#L90

@vweevers
Copy link
Member Author

vweevers commented Jan 27, 2019

@vweevers
Copy link
Member Author

vweevers commented Jan 27, 2019

To fix android-arm, we need to add a GYP_DEFINES environment variable with value target_arch=arm android_target_arch=arm host_os=linux OS=android to prebuildify-cross or yet-to-create docker images or prebuildify (as long as the env var ends up in node-gyp and thus gyp). Same for arm64.

Note:

  • OS must be uppercase
  • target_arch might not be necessary, because we're also passing an ARCH

@vweevers vweevers changed the title Add linux-arm64, android-arm and android-arm64 prebuilds Add linux-arm64, android-arm and android-arm64 prebuilds [wip] Jan 27, 2019
@ralphtheninja
Copy link
Member

@vweevers We got a segmentation fault in https://travis-ci.org/Level/leveldown/jobs/485175594#L1623 which is a bit worrying (nothing to do with the cross compiled stuff).

@ralphtheninja
Copy link
Member

ralphtheninja commented Jan 28, 2019

Gah. Travis borked of course. Restarting builds.

@ralphtheninja ralphtheninja changed the title Add linux-arm64, android-arm and android-arm64 prebuilds [wip] Add linux-arm64, android-arm and android-arm64 prebuilds Jan 28, 2019
Copy link
Member

@ralphtheninja ralphtheninja left a comment

Choose a reason for hiding this comment

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

Approving my own changes :D

@vweevers
Copy link
Member Author

We got a segmentation fault in https://travis-ci.org/Level/leveldown/jobs/485175594#L1623 which is a bit worrying (nothing to do with the cross compiled stuff).

Can you open an issue? Let's hope we can replicate + fix before 5.0.0

@vweevers
Copy link
Member Author

I'll review later

#!/bin/bash

DOCKER_USER="node"
if [[ "$TRAVIS" == "true" ]]; then DOCKER_USER="travis"; fi
Copy link
Member Author

Choose a reason for hiding this comment

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

@ralphtheninja Can you explain the users? I'm guessing the working directory on the host is owned by travis, and docker must use the same user? Can't we change permissions instead?

Copy link
Member

Choose a reason for hiding this comment

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

I'm open to suggestions. If we don't do anything about the user (e.g. set USER in the Dockerfile) the container will run as root, thus producing binaries owned by root. Which leaves us with two options (as I see it):

  1. either we do something related to user, i.e. run the container with the correct user
  2. keep running the container as root, but copy out the binaries from the container, which is done in prebuildify-cross

Copy link
Member

Choose a reason for hiding this comment

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

Thinking about it some more, I think I prefer 2. since the docker image doesn't need any specific users. This needs some tweaking to the cross-compile script as well. Also need to look into how prebuildify-cross does this with output folders etc.

Copy link
Member Author

Choose a reason for hiding this comment

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

I liked the initial approach of running as the node user. Why did that not work?

Copy link
Member

Choose a reason for hiding this comment

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

Because it runs as user with uid 1000 and the jenkins user is 2000.

Copy link
Member Author

@vweevers vweevers Feb 1, 2019

Choose a reason for hiding this comment

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

Because it runs as user with uid 1000 and the jenkins user is 2000.

*travis user? 😉

Alright, I'm fine with both the current solution and the alternative, copying out the binaries. The latter would also remove the npm i problem (#587 (comment)), because if we copy out the binaries then we don't have to mount any volume. ignore me, it's late

Copy link
Member

@ralphtheninja ralphtheninja Feb 1, 2019

Choose a reason for hiding this comment

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

Yes, travis user. I don't know where jenkins came from. Jenkins! Get out of my head!

Copy link
Member

Choose a reason for hiding this comment

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

It's not a perfect solution, but it works. Lets tweak it as we go?

scripts/cross-compile Outdated Show resolved Hide resolved
if [[ "$TRAVIS" == "true" ]]; then DOCKER_USER="travis"; fi

exec docker run -u ${DOCKER_USER} --rm -it -v $(pwd):/app prebuild/${IMAGE} \
bash -c "npm i --ignore-scripts && npx prebuildify -t 8.14.0 --napi --strip"
Copy link
Member Author

Choose a reason for hiding this comment

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

  1. Seeing as each invocation of this script runs in the same working directory, they all work against the same node_modules, and reinstall each time. Can we A) skip that or B) mount node_modules as a named volume, so that changes in the docker guest are hidden from the host?
  2. Does npx use a locally installed bin if available, or does it always install the latest version of prebuildify?

Copy link
Member

@ralphtheninja ralphtheninja Feb 1, 2019

Choose a reason for hiding this comment

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

Can we A) skip that

Not sure what you mean by that. Unsure what you mean here, mind writing snippet that explains the changes you want to make?

Afaik, npx uses a locally installed .bin if available. If it didn't, we wouldn't be able to use the specific branch of prebuildify we need to override e.g. platform.

Copy link
Member Author

@vweevers vweevers Feb 1, 2019

Choose a reason for hiding this comment

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

Not sure what you mean by that.

npm i.

Unsure what you mean here, mind writing snippet that explains the changes you want to make?

I'll elaborate. We can either A) skip npm i in docker. We simply reuse what's already installed on the host. That only works (safely) in Travis though, where both host and guest are Linux. Not when testing things locally (on Mac, Windows or a different Linux flavor). That's also why the current approach is less than ideal, because after npm run prebuild-arm, you're stuck with a node_modules meant for a different platform.

So option B is to use a named volume. An anonymous volume may also work. Something like docker run -v $(pwd):/app -v :/app/node_modules. Initially, Docker will copy the existing /app/node_modules into the second volume. From then on, any changes to node_modules (e.g. by npm i) are applied to the second volume, meaning they don't affect node_modules on the host.

Afaik, npx uses a locally installed .bin if available. If it didn't, we wouldn't be able to use the specific branch of prebuildify we need to override e.g. platform.

👍

@vweevers
Copy link
Member Author

vweevers commented Feb 1, 2019

I can't approve my own PR so LGTM

@ralphtheninja ralphtheninja merged commit f1d4c7c into master Feb 1, 2019
@ralphtheninja ralphtheninja deleted the prebuild-arm-flavors branch February 1, 2019 22:00
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Determine for which ARM flavors we want prebuilds Cross compiling for ARM on travis?
2 participants