-
Notifications
You must be signed in to change notification settings - Fork 355
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
cross-platform, cross-compile build toolchain #280
Conversation
- added CMake scripts with build support for Windows/Visual Studio - improved V8Locker messages (shows the conflicting thread names) - removed Visual Studio solution & project files
- NodeJS is expected to be in a co-located "node" directory by default
- also reverted changes to V8Locker exception messages introduced in 9686139
- use environment variables in maven pom.xml - remove former MacOS shell scripts
detailed notes for all the fixes: - [x] build context class (arch, cwd, platform-cfg, custom step options) - [-] macos/android c++11 - [-] macos/android disable g++ warnings - [x] android make sure g++ output is shown (remove pipe to /dev/null) - [x] gradlew on windows - [x] pass CLI params in build-context (used in android build) - [-] rework inject_env API to use it outside the platform-config - [x] make j2v8junit a separate build step in all platforms - [x] make a central constant collection for common build commands - [x] use immutable data structures for build context - [x] move pre-build checks & setup to build-instigator instead of build-executor - [x] get cwd for build only once and pass it along (immutable) - [x] make MSC++RT static vs. dynamic linking a cmake option (default is dynamic) - [x] move -fPIC to build commands, away from Dockerfiles or any other places - [x] used shared shell scripts in android dockerfile to install dependencies - [x] reorder android emulator integration in Dockerfile - [x] set up env in vagrant not pyton for MacOS - [x] dont copy native lib in cmake, but clear src/main/libj2v8* and copy only platform specific lib - [x] document build system & options - [x] do not rely on bash/linux utils being installed on windows - [x] add windows batch build system if necessary (or platform switch case in shell-build-system) - [~] windows docker support - [x] add check if win32 is built with cross-compile and show "not supported" error & hints about PDB problem
- common build settings managed in central file - added support for basic win32 cross-compiling - added workaround for win32 docker msbuild PDB bug - extended build_utils.store_nodejs_output to cache additional win32 output directories - added script for cloning nodejs git repo & apply provided patches - added script to store current changes to node into patch file - moved functions for executing CLI commands to build_utils - added additional docker health-checks if the docker server is using the right platform (linux vs. windows containers) - fixed maven download URLs (were expired) - added node v7.4.0 patch (includes win32 docker PDB fixes)
- fixed setEnvVar OS semantic bug - added missing copyNativeLibs support for Android AAR packaging - file_abi (file_arch) settings are now explicitely stored with the target platform
# Conflicts: # build.gradle # jni/com_eclipsesource_v8_V8Impl.cpp # pom.xml - added additional info when builtin node-modules are not defined for J2V8
Lets hope @irbull has time to review this and put pertinent comments in places. it is a biggie -> this PR. |
@drywolf can you provide some documentation on how to use what you wrote? |
@matiwinnetou Sure, I already put some hints into the README.md ... what I think could be useful to understand is the individual build steps that are performed and what they produce, I will write down some notes on that. Any hints on what else would be worth documenting ? |
Thanks so much! I have a bunch of family things going on this weekend, but I will try to get to this. I think I'll cut a 4.8.0 release with what we have, just to get that our the door, then I'll pull all this in. @drywolf If you send me your address (my githubhandle at gmail.com) I'll send you some J2V8 stickers I had made. |
Follow these steps to build J2V8 from source: | ||
|
||
1) clone the Node.js source code | ||
- `python prepare_build.py` |
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.
It is worth to mention that this works only for python2 (tried with python3 but doesn't work), was there any reason why you choose python over lets say bash scripts? I am just thinking out loud here that not everyone has python2 installed.
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.
also when I already had node folder checked out the build failed:
[mati@winnetou-pc J2V8]$ python2 prepare_build.py
Node.js is already cloned & checked out
Traceback (most recent call last):
File "prepare_build.py", line 20, in <module>
branch = utils.get_node_branch_version()
File "/home/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 14, in get_node_branch_version
out = execute_to_str("git branch", "node")
File "/home/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 40, in execute_to_str
p = subprocess.Popen(cmd, stdout=subprocess.PIPE, cwd=cwd)
File "/usr/lib/python2.7/subprocess.py", line 390, in __init__
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1024, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
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.
even most simple build didn't work on my latest manjaro (arch) linux:
python2 build.py -t linux -a x64
most likely I ran into this issue: gcc7 doesn't compile:
make[1]: *** [deps/v8/src/v8_base.target.mk:554: /home/mati/Devel/OpenSource/J2V8/node/out/Release/obj.target/v8_base/deps/v8/src/heap/heap.o] Error 1
make[1]: *** [deps/v8/src/v8_base.target.mk:554: /home/mati/Devel/OpenSource/J2V8/node/out/Release/obj.target/v8_base/deps/v8/src/heap/mark-compact.o] Error 1
make[1]: Leaving directory '/home/mati/Devel/OpenSource/J2V8/node/out'
make: *** [Makefile:73: node] Error 2
Traceback (most recent call last):
File "build.py", line 311, in <module>
execute_build(args)
File "build.py", line 307, in execute_build
execute_build_step(target_compiler, target_step)
File "build.py", line 266, in execute_build_step
compiler.build(build_step)
File "/home/mati/Devel/OpenSource/J2V8/build_system/cross_build.py", line 60, in build
self.exec_build(config)
File "/home/mati/Devel/OpenSource/J2V8/build_system/shell_build.py", line 26, in exec_build
self.exec_cmd(shell_str, config)
File "/home/mati/Devel/OpenSource/J2V8/build_system/cross_build.py", line 79, in exec_cmd
utils.execute(cmd, dir)
File "/home/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 37, in execute
raise subprocess.CalledProcessError(return_code, cmd)
subprocess.CalledProcessError: Command 'cd /home/mati/Devel/OpenSource/J2V8 && cd ./node && ./configure --without-intl --without-inspector --dest-cpu=x64 --without-snapshot --enable-static && CFLAGS=-fPIC CXXFLAGS=-fPIC make -j4' returned non-zero exit status 2
this makes me think that perhaps docker should always be used...
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.
was there any reason why you choose python over lets say bash scripts? I am just thinking out loud here that not everyone has python2 installed.
There are several reasons why I would not use bash scripts for more complex scenarios
- the composability of bash/shell is relatively limited compared to python's (functional reuse, data reuse, ...)
- bash is only natively available on MacOS / Linux environments (python covers all platforms and also delivers platform independent APIs for OS functions, e.g. file access)
- IDE code maintenance & refactoring support
I targeted Python 2 primarily because you already need it if you want to build Node.js from source. But I will look into how much work it would take to make it compatible for Python 3 (but I don't think this will be a priority, since Python 2 is still very widely adopted)
About the error you get from running prepare_build.py
... this is mainly a utility script that is intended to get you started from a fresh checkout of the J2V8 repository. For the case when there is already a node
directory checked out this script should not try to override anything you already have there, but I will add a check for that and print some more helpful error message, thanks for pointing this out.
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.
You did so much amazing work I have to say. I think you have a point here, I was just thinking from a scenario that one does git clone and invokes command to build, the rest would happen in docker container. One of biggest advantages of docker as I understood this is to eliminate various linux distrobution flavours, e.g. gcc version, kernel version, different packages versions, python2 vs python3. In that respect it would be perhaps better to fork of python2 invoking a build inside a docker container (in theory) -> that way probability that build would work is higher.
On another hand, if this is intended to be used by Travis to create releases (as most people may not care to build snapshots), then it doesn't really matter. As long as maintainers of this project know how to run this, then it is fine.
self.exec_host_cmd("docker stats --no-stream", config) | ||
|
||
# NOTE: the additional newlines are important for the regex matching | ||
version_str = utils.execute_to_str("docker version") + "\n\n" |
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.
[mati@winnetou-pc J2V8]$ git:(𝘮 2 ⇄ 23 drywolf-master)python build.py -t linux -a x64 -ne -x
Checking Node.js builtins integration consistency...
Caching Node.js artifacts...
>>> Used existing Node.js build files: linux.x64
CONTAINER CPU % MEM USAGE / LIMIT MEM % NET I/O BLOCK I/O PIDS
Traceback (most recent call last):
File "build.py", line 311, in <module>
execute_build(args)
File "build.py", line 279, in execute_build
execute_build_step(x_compiler, x_step)
File "build.py", line 266, in execute_build_step
compiler.build(build_step)
File "/home/mati/Devel/OpenSource/J2V8/build_system/cross_build.py", line 47, in build
self.health_check(config)
File "/home/mati/Devel/OpenSource/J2V8/build_system/docker_build.py", line 23, in health_check
version_str = utils.execute_to_str("docker version") + "\n\n"
File "/home/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 40, in execute_to_str
p = subprocess.Popen(cmd, stdout=subprocess.PIPE, cwd=cwd)
File "/usr/lib/python2.7/subprocess.py", line 390, in __init__
errread, errwrite)
File "/usr/lib/python2.7/subprocess.py", line 1024, in _execute_child
raise child_exception
OSError: [Errno 2] No such file or directory
[mati@winnetou-pc J2V8]$ git:(𝘮 2 ⇄ 23 drywolf-master)docker version
Client:
Version: 17.05.0-ce
API version: 1.29
Go version: go1.8.1
Git commit: 89658bed64
Built: Fri May 5 22:40:58 2017
OS/Arch: linux/amd64
Server:
Version: 17.05.0-ce
API version: 1.29 (minimum version 1.12)
Go version: go1.8.1
Git commit: 89658bed64
Built: Fri May 5 22:40:58 2017
OS/Arch: linux/amd64
Experimental: false
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.
is this a bug?
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.
could you please try adding , universal_newlines=True, shell=True
to line 40 of build_utils.py
:
p = subprocess.Popen(cmd, stdout=subprocess.PIPE, universal_newlines=True, shell=True, cwd=cwd)
I think that's what is missing there, once you can confirm the fix I will add it to the PR
Thanks
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.
sure, I will
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.
It did help and I ran into a new issue:
[mati@winnetou-pc J2V8]$ git:(𝘮 2 ⇄ 23 drywolf-master) 1Mpython build.py -t linux -a x64 -ne -x
Checking Node.js builtins integration consistency...
Caching Node.js artifacts...
>>> Used existing Node.js build files: linux.x64
CONTAINER CPU % MEM USAGE / LIMIT MEM % NET I/O BLOCK I/O PIDS
Error response from daemon: No such container: j2v8.linux.x64
preparing linux@x64 => cross-compile-host
Sending build context to Docker daemon 46.59kB
Step 1/17 : FROM philcryer/min-jessie:latest
---> 7876bd1070da
Step 2/17 : RUN mkdir -p /temp/docker/shared/
---> Running in d42df0af88a6
---> 647358ea8deb
Removing intermediate container d42df0af88a6
Step 3/17 : WORKDIR /temp/docker/shared/
---> ef3fbce6d142
Removing intermediate container 6156826e7560
Step 4/17 : COPY ./shared/install.debian.packages.sh /temp/docker/shared
---> 75582a21fab6
Removing intermediate container 72275340d1ba
Step 5/17 : RUN ./install.debian.packages.sh
---> Running in f49aef583c98
/bin/sh: 1: ./install.debian.packages.sh: Permission denied
The command '/bin/sh -c ./install.debian.packages.sh' returned a non-zero code: 126
Traceback (most recent call last):
File "build.py", line 311, in <module>
execute_build(args)
File "build.py", line 279, in execute_build
execute_build_step(x_compiler, x_step)
File "build.py", line 266, in execute_build_step
compiler.build(build_step)
File "/home/mati/Devel/OpenSource/J2V8/build_system/cross_build.py", line 59, in build
self.pre_build(config)
File "/home/mati/Devel/OpenSource/J2V8/build_system/docker_build.py", line 49, in pre_build
self.exec_host_cmd("docker build -f $PLATFORM/Dockerfile -t \"j2v8-$PLATFORM\" .", config)
File "/home/mati/Devel/OpenSource/J2V8/build_system/cross_build.py", line 70, in exec_host_cmd
utils.execute(cmd, dir)
File "/home/mati/Devel/OpenSource/J2V8/build_system/build_utils.py", line 37, in execute
raise subprocess.CalledProcessError(return_code, cmd)
subprocess.CalledProcessError: Command 'docker build -f linux/Dockerfile -t "j2v8-linux" .' returned non-zero exit status 126
[mati@winnetou-pc J2V8]$ git:(𝘮 2 ⇄ 23 drywolf-master) 1M
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.
Ok, looks like a Linux execute permission problem, can you please try running chmod +x
for all shell files in ./docker/shared
. I think this will fix this issue, I will see how I can make this automated.
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.
This did the trick, can you add this somewhere to pyton script or git repo that that those scripts have to be executable?
Now building is going further
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.
seems to have worked
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 please commit the fix for:1
- chmod +x docker/shared/*
- build_utils.py:40 -> p = subprocess.Popen(cmd, stdout=subprocess.PIPE, universal_newlines=True, shell=True, cwd=cwd)
Sounds good 👍 let me know if you need anything that I can help with.
Thanks, I'm very much appreciating the offer of sticker-swag 😄 (I sent you an e-mail) |
mac osx build failed on Sierra:
|
docker/linux/Dockerfile
Outdated
@@ -0,0 +1,31 @@ | |||
FROM philcryer/min-jessie:latest |
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.
if you would use ubuntu:zesty like I just did you would not have to install extra cmake but correct cmake is available in ubuntu:zesty.
docker pull ubuntu:zesty
docker run -ti ubuntu:zesty /bin/bash
apt-get update && apt-get upgrade -y
apt-get install git vim python cmake gcc g++ default-jdk maven
mkdir build
cd build
git clone https://github.com/drywolf/J2V8
export JAVA_HOME="/usr/lib/jvm/java-8-openjdk-amd64"
python prepare_build.py
python build.py -t linux -a x64
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.
With my manjaro (arch) I had to resort to compile like this (without using docker scripts from you). As it turns out the current build doesn't support yet latest gcc (7)
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.
Sorry for not replying earlier, I was busy with IRL stuff. Thanks for the testing & feedback ... I will work those fixes / improvements into the PR 👍
please rebase, there are conflicts. |
The conflicts are pretty easy to fix, it's just related to the version numbers. I took this for a spin last night, and after fixing the errors that @matiwinnetou found, I was able to build this (on Mac) for Linux. I will try the Android and Mac builds today. This doesn't help with cross-platform builds with a Windows target, as you cannot run a Windows Container on anything except a Windows host, but the CMake additions make this much better than it was before. I will dig up a windows machine to test this there too. Thanks again @drywolf, and thanks to @matiwinnetou for taking a look through this too. |
- added BUILDING.md describing the individual build steps - now the docker builds use ubuntu:zesty as base image - permission fixes for docker shell files - build_utils fix
# Conflicts: # build.gradle # pom.xml
The fixes discussed above are now committed. Currently on my TODO list there are only the following points open:
Where the third point is mostly a backlog item, until I get some feedback either from Microsoft or the Docker for Windows community (docker/for-win#829)
That's right. We could look into also supporting a Vagrant windows target for this case. I already thought about doing that, but for Windows I'm not sure if there's a freely available base VM image that one could use with Vagrant (VirtualBox). If there is such an image, then the additional support for that would be trivial to add now 😄 |
__Inputs:__ | ||
- Node.js source code | ||
- see [Github](https://github.com/nodejs/node) | ||
- Node.js GIT patches with customizations for integrating Node.js into J2V8 |
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.
good to know, so building with another NodeJs is not out of the box. I didn't know one needed such patches (although I believe there was a legitimate reason).
👍
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.
This is more or less there for convenience / to be used if necessary ... I know there has been work in other issues / PRs to get the build working without any customization to node (which would be preferred of course), but until we get there, this was a clean way that I added to overcome any such issues in the meantime.
- `./cmake/*.cmake` | ||
|
||
__Artifacts:__ | ||
- CMake generated Makefiles / IDE Project-files |
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.
wow, this documentation is so awesome 👍
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.
Thanks .:heart:
I wanted to add more but my time was limited. I will continue documenting what I think would be helpful once I have some more spare time.
Wow! This is just awesome! Really looking towards this getting merged. I did work on a alpine compile target in regards to #252 last week. I will port it to this new approach and give feedback if I stumbled across anything. |
@kernle32dll interesting usecase, I didn't know that Alpine used a different stdlib. |
@drywolf yes, sadly both Node.js and J2V8 have to be compiled inside a Alpine environment. There is currently - at least to my knowledge - no way to cross-compile for musl. It might not even be feasible in general, because of linker hell with such a low level dependency. Trust me, this caused much grief beyond J2V8 in our project already :/ But yeah, what I did was (with the old, pre-pr approach) create a docker container in both the J2V8 and J2V8 static-build, to output the appropriate files. This did work, and all maven tests ran successfully after some small java code adjustments to differentiate between linux and alpine libs (might not be necessary). The code adjustments were necessary, as java cannot differentiate between linux and alpine, because its the same "system", but just a different stdlib. My adjustment was just some hindsight to enable multiple, multi-plattform libs in one maven dependency (see #257). I will push my code to my fork tomorrow, so you can have a look. |
@drywolf Successfully integrated an alpine flavor with your build system. Works like a charm! Edit: One thing to note is that I used openjdk instead of oracle jdk to build. Two reasons: 1) it is not possible to get it cleanly running on Alpine (hard glibc dependency, there are some hacky solutions) and b) licence concerns with oracle. Maybe it is a solution to use the java docker image as a base for the other builds, too? (openjdk:8-jdk then). You can read up on the legal part here. |
@kernle32dll Cool stuff ❗️ 👍 Easy extensibility is one of the major goals I had hoped for and what I tried to optimize the architecture of the system for. So I'm really happy to see that you could already integrate your usecase in a convenient way 😄 About the question of JDK vendors ... if it would even further reduce the complexity of the Dockerfiles and reduze the produced docker image sizes, then I would be all for changing it. For the legal questions @irbull needs to decide what is more appropriate for the further developments of the project. |
Based on the two scenarios that already came up for additional It would get rid of the // building without cross-compile stays the same
python build.py -t win32 -a x64 -ne
// old cross-compile (would be removed)
python build.py -t win32 -a x64 -ne -x
python build.py -t win32 -a x64 -ne --cross-compile
// new default cross-compile (would use the designated default cross-compile environment)
python build.py -t win32:x -a x64 -ne
python build.py -t win32:cross-compile -a x64 -ne
// support for different cross-compile environments, but targeting the same binary platform
python build.py -t win32:docker -a x64 -ne
python build.py -t win32:vagrant -a x64 -ne
// support for alternative flavors of otherwise pretty much identical platforms
python build.py -t linux -a -x64 -ne
python build.py -t linux:alpine -a -x64 -ne I'm just beginning to work on this change, so any feedback / additional ideas are welcome 😉 |
4.8.0 Build on macosx fails for me:
|
vargant build seems to be running though on osx and linux (manjaro). Currently downloading a large osx.sierra image.
Did you check this on OSX at home that a non --cross-compile build works natively on osx? |
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.
This is great work! I spent yesterday reading up on cmake and today I went through each commit. I left some small comments that I think we should address. Also a few big picture comments here:
-
To make this practical, we need a way to use / reuse a pre-built node.js. I saw that this will use one if it exists from a previous run of the the build, but on a build server with a fresh workspace, a prebuilt version won't be there. It can take up to 1 hour to build node.js, and travis-ci will timeout long before that. If we provide a prebuilt node.js, the build script can D/L that. For people who want to build everything from scratch, we can support that too.
-
We should add some overview to each of the .py scripts, especially the ones at the root. I'm not sure what each one does, or if users are expected to execute them.
-
There appears to be lots of commented out code in some of the scripts, especially the
build_all.py
. We should remove commented out code.
Having said that, I think it's best we commit this PR and address any questions in further commits.
Thanks again for everyone involved, especially @drywolf
CMakeLists.txt
Outdated
option(J2V8_BUILD_ONLY_DEBUG_RELEASE "Generate only Debug and Release configurations (exclude RelWithDebInfo and MinSizeRel)" ON) | ||
|
||
# set up the module path | ||
set (CMAKE_MODULE_PATH ${CMAKE_SOURCE_DIR}/cmake) |
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.
This appears to be a duplicate line. See line 5.
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.
Thanks, good catch ... i will clean this one up in the upcoming PR
CMakeLists.txt
Outdated
|
||
# enable Node.js if requested by the set options | ||
if (J2V8_NODE_COMPATIBLE) | ||
#{ |
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.
Why are curly braces commented out?
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 use those just by convention to make it easier for the eye to quickly read where the scope of a if-elif-else block starts and ends. This being said, for the next PR I have removed those where only a single line is inside the branch scope. From now on they are only used for if-elif-else blocks that contain multiple lines in their respective blocks (which was the original intent). Thanks for the feedback
@@ -0,0 +1,14 @@ | |||
|
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.
We should include some docs describing what this does. I don't know what is meant by policy here, and I don't know what these policies (CMP0008 for example) are.
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 will add links to the corresponding CMake documentation. I added them because if you don't then on some platforms CMake will print a lot of warnings while being invoked on the CMakeLists.txt
These statements strictly define the behavior for what those rules are supposed to do in CMake and get rid of those warnings.
build_all.py
Outdated
import build_platform as b | ||
|
||
# b.execute_build(b.target_macos, b.arch_x86, b.build_all, node_enabled = True, cross_compile = True) | ||
b.execute_build(b.target_macos, b.arch_x64, b.build_all, node_enabled = True, cross_compile = True) |
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.
Isn't node_enabled
set in the cmake
file? Why is it set here too?
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 way command-line switches and arguments are translated to changes in the produced binary build artifacts is (roughly) as follows:
--node-enabled
--> params.node_enabled
--> J2V8_NODE_ENABLED
--> NODE_COMPATIBLE=1
--node-enabled
// passed via the build.py CLI- is parsed in the python code and turned into a boolean variable + value
params.node_enabled
// variable in python containing the parsed boolean value- is put into a "config" dictionary/object that will be passed as a parameter to each build-step function for the build
- each build-step can decide to access the variable and do something with it
J2V8_NODE_ENABLED
// the CMakeLists.txt file exposes this boolean variable that can be overridden when invoking the cmake CLI- the
def build_j2v8_cmake(config):
build-step passes the value of the "config" object propertynode_enabled
to the cmake CLI when it is invoked in this build-step
- the
NODE_COMPATIBLE=1
// if the above CMake variable is set "TRUE"- this compiler preprocessor-definition will be included when generating the native build files
Having said that, the above code of build_all.py
is just using the way to skip over the first step of the chain shown above, and directly pass a user-specified configuration to the python function that then continues to process the build in the same way as if it was originally called from the CLI.
PS: in the next PR the code from build_all.py
will be moved to build_configs.py
build_all.py
Outdated
# b.execute_build(b.target_macos, b.arch_x86, b.build_all, node_enabled = True, cross_compile = True) | ||
b.execute_build(b.target_macos, b.arch_x64, b.build_all, node_enabled = True, cross_compile = True) | ||
|
||
# b.execute_build(b.target_linux, b.arch_x64, b.build_all, True, True) |
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.
Why are most of the lines in this file commented out?
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 used this file mostly for testing during the early build-system development, in the next PR this will be more fleshed out and become a feature to run an interactive CLI and choose from a set of pre-configured build-configurations
@@ -39,12 +39,12 @@ dependencies { | |||
} | |||
|
|||
android { | |||
compileSdkVersion 10 | |||
compileSdkVersion 19 |
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.
Any reason why 19? We should try to keep this as low as possible.
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 think at some point while I worked on how I could get the Android emulator testing integration working, it was required by some dependency that might now be gone. If the build & tests work fine with version 10 then we can safely revert it back
docker/android/Dockerfile
Outdated
RUN mkdir /usr/local/android-sdk/tools/keymaps && \ | ||
touch /usr/local/android-sdk/tools/keymaps/en-us | ||
|
||
EXPOSE 22 |
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.
Why do we need all these ports exposed in the docker container?
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 got those parts from this Dockerfile running the Android emulator.
The ports expose the ADB for interaction with the emulator from the outside, and the QEMU VNC port (you can connect to the emulator using a VNC client and see the Android screen, touch inputs do not work though)
import sys | ||
import shutil | ||
|
||
# this is a cross-platform polyfill for "cp" |
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.
A polyfill for rm?
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.
Thanks, the typo will be fixed in the next PR
I could just test it within the same VirtualBox VM that is also used by the cross-compile, but there I tried a clean git clone of my J2V8 fork -> prepare_build.py -> build.py and it worked. I can't test it on a real Mac since I don't have access to one. The error you posted above suggests that there is something going wrong along those lines in the MacOS cmake build step: u.shell("mkdir", "cmake.out/$PLATFORM.$ARCH") + \
["cd cmake.out/$PLATFORM.$ARCH"] + \ Can you please verify if there are the |
Thanks @irbull for the review & merge. I will have a look at the detailed comments tomorrow.
The built / prebuilt node binaries are expected to be located in
Agreed. I will start work on this tomorrow.
I like your emphasis on that 😄 I'm usually pretty nitpicky when it comes to commented out code too. I hope I did not forget to remove too much of dead code. For the |
|
This adds an easy-to-use, automated build toolchain that should replace the previous shell scripts and manual labor that is necessary to produce a working & tested build for all of the platforms & architectures that J2V8 should support.
see #232 and #261
Note: I left most of the existing shell scripts and Dockerfiles in place, but once this PR is merged we should go ahead and clean up everything that is deprecated by this PR.
Thanks