additional Build-System features & fixes #327
Conversation
- moved check_node_builtins to build_utils - added --no-shutdown option for vagrant builds - fixed typo in JNI lib output name on win32 (missing lib prefix) - refactored all existing platform configs to align with new build-env APIs - added graceful handling / shutdown of build-envs if user cancels the build early - added powershell scripts for build dependencies - corrected usage of "smb" mounted folders for macos:vagrant target - added win32:vagrant target
- register atexit events before cross-builds are started - switched linux & android docker images to Ubuntu Xenial LTS (see eclipsesource#311) - cleaned up and documented switch-to-winrm-plugin that is used in win32:vagrant builds
Features ------------------------ [Build-System] - replaced default Android instrumentation test runner with Spoon test runner (allows to optionally run individual tests on emulator) - added interactive CLI mode (see build.py, build_interactive.py) - added "--interactive, -i" CLI parameter to start interactive build CLI - added support for specifying "anti-build-steps" via the CLI (for example: "all ~nodejs ~test" runs all steps, except for "nodejs" and "test") - added "--vendor, -v" CLI parameter that allows to build specialized linux vendor binaries, e.g. for alpine-linux - added alpine-linux binary support - added "inject_env" API that is directly callable on a build-step config - added "--sys-image, -img" CLI parameter that can be used to override the used OS image for Docker / Vagrant builds - refactored the original "--cross-compile, -x" parameter into two separate parameters "--docker, -dkr" and "--vagrant, -vgr" (this makes it possible to support both variants for all target platforms) - added "--keep-native-libs, -knl" CLI parameter (can be useful for building bundles that should include multiple platform binaries) - centralized some repeated constant literals & code from build-steps across multiple platforms (into build_utils.py, cmake_utils.py and shared_build_steps.py) - added automatically set "file_abi" property on build-step configs - added more variables for use with "inject_env" API - added "adb logcat" process to Android Docker build (writes realtime output from Android emulator to ./docker/android/logcat.txt) - moved all Node.js utility code to nodejs.py CLI script - added experimental code to package Node.js pre-built binary packages (see nodejs.py) [J2V8] - updated LibraryLoader code to include Linux-Vendor specifier when looking for native libs (vendor-specific libs are preferred over vendor-agnostic ones) - added PlatformDetector as a central place to get normalized string identifiers for OS,Arch,Vendor Fixes & Changes ------------------------ [Build-System] - reorganized some code of the build-system into more separate files (see build.py, build_constants.py, build_executor.py, cli.py) - added missing "bool" type to immutable.py - throw error in CMake scripts if Java-Home could not be found - default Docker base-image for Linux & Android builds is now "debian:jessie" - installation of JDK in install.jdk.sh now checks if a JDK is already installed (needed for alpine-linux build) - refactored win32 Dockerfile (now uses the same PowerShell scripts as the Vagrant build instead of inline commands) [J2V8] - separated LibraryLoader and Platform-Detection code (see PlatformDetector.java) - show native lib-name in IllegalStateException message when native lib could not be loaded - AllTests.java is now called A_RunAheadTests and only includes the V8RuntimeNotLoadedTest (this is the only test that should be run before any other tests) - extended and refactored LibraryLoaderTest / PlatformDetectorTest implementations - added timeout for V8LockerTest - added workaround for V8RuntimeNotLoadedTest (is currently not implemented to run on Android)
- added build-util that directly sets the required build variables into the pom.xml file without using environment variables - reverted gradle sdk- & target-versions back from 19 to 10 - added alias-functions for the polyfill shell commands to shared_build_steps.py - started documenting the build-system modules & functions - fixed git EOL problems for shell/python scripts used by Docker builds
|
@matiwinnetou As promised 😉
My Roadmap for when this PR is done
|
BUILDING.md
Outdated
|
|
||
| usage: build.py [-h] --target {android,linux,macos,win32} --arch {x86,x64,arm} | ||
| [--vendor VENDOR] [--keep-native-libs] [--node-enabled] | ||
| [--docker] [--vagrant] [--sys-image SYS_IMAGE] [--no-shutdown] |
There was a problem hiding this comment.
I don't see -v in the list of description here...
There was a problem hiding this comment.
-v is just the short form for the --vendor switch, I haven't yet seen a way to show both in the python args parser generated help
BUILDING.md
Outdated
| [0] Docker >> android-x86 >> NODE_ENABLED | ||
| [1] Docker >> android-arm >> NODE_ENABLED | ||
| [2] Docker >> alpine-linux-x64 >> NODE_ENABLED | ||
| [3] Docker >> linux-x64 >> NODE_ENABLED |
There was a problem hiding this comment.
Why did you choose these ones as suggestions? Is it possible to add suggestions? I though interactive would be that I would pick target, vendor, platform, etc, interactively.. also I found a tiny bug, it is -i or --interactive, --i doesn't work
There was a problem hiding this comment.
Why did you choose these ones as suggestions?
I though interactive would be that I would pick target, vendor, platform, etc, interactively..
Because not all combinations of platforms * architectures * vendors * virtualization-tech * feature-variations[N] are currently technically possible nor would it be sensible to try and support them all.
For the current suggestions I added the ones that are working and that were important for me to test the cross-platform build.
Is it possible to add suggestions?
Yes, have a look at build_system/build_configs.py
also I found a tiny bug, it is -i or --interactive, --i doesn't work
You can use -i or --interactive ... they are both doing the same thing (they are aliases)
There was a problem hiding this comment.
That being said, I also can imagine many ways to improve upon this feature and see what would be the best DX for the interactive CLI for devs that want to build J2V8 themselves.
But that would be stuff for a next PR ... this one is already bloated enough 😆
BUILDING.md
Outdated
| [build-steps [build-steps ...]] | ||
| ``` | ||
| ``` | ||
| python build.py -v alpine -t linux -a x64 -dkr -img openjdk:8u131-alpine -ne j2v8 |
There was a problem hiding this comment.
I know it sounds silly but I would suggest to put more examples here for poor souls that try to understand params.
BUILDING.md
Outdated
| # Build-Steps | ||
|
|
||
| The J2V8 build-system performs several build steps in a fixed order to produce the final J2V8 packages for usage on the designated target platforms. What follows is a short summary for what each of the executed build-steps does and what output artifacts are produced by each step. | ||
|
|
||
| --- | ||
| ## Node.js | ||
|
|
||
| Builds the [Node.js](https://nodejs.org/en/) & [V8](https://developers.google.com/v8/) dependency artifacts that are later linked against by the J2V8 native bridge code. | ||
| Builds the [Node.js](https://nodejs.org/en/) & [V8](https://developers.google.com/v8/) dependency artifacts that are later linked into the J2V8 native bridge code. |
There was a problem hiding this comment.
I feel guilty about commenting that you did so much work but I have to say I didn't learn much from description from nodejs.py:
➜ J2V8 git:(drywolf-build-sys-features) ✗ python nodejs.py -h
usage: nodejs.py [-h] command
positional arguments:
command
optional arguments:
-h, --help show this help message and exit
I still don't know how checkout nodejs source, it used to be python prepare_build.py
There was a problem hiding this comment.
I had to breat something to get commands:
➜ J2V8 git:(drywolf-build-sys-features) ✗ python nodejs.py 7.4.0
usage: nodejs.py [-h] command
nodejs.py: error: argument command: invalid choice: '7.4.0' (choose from 'flush-cache', 'fc', 'git-init', 'gi', 'package', 'pkg', 'store-diff', 'sd', 'apply-diff', 'ad')
There was a problem hiding this comment.
This script is far from finished, I need to first get a better picture of how this would be used in a CI build before I start this kind of polishing 😉
But I agree that this help output is pretty useless even at this point, I will add some preliminary help for now.
PS: Thanks for reviewing ... once you are done I will start to see what needs fixing and how to prioritze 👍
PPS: looks like there is a feature of the args parser that I could use to improve upon that
|
I like store patch in nodejs.py little helper but not really sure what package function does, when would one use it? I am trying to do some QA here as well: It worked, except for unit tests I think NodeJS unit tests failed because I didn't build this with NodeJS support? It produced jar file: though |
nodejs.py
Outdated
| import os | ||
| import sys | ||
| import tarfile | ||
| import zipfile |
There was a problem hiding this comment.
this is showing in my IDE as not needed..., but maybe it is...
| import argparse | ||
| import collections | ||
| import fnmatch | ||
| import glob |
There was a problem hiding this comment.
this is showing in my IDE as not needed..., but maybe it is...
|
Dockerized build for linux didn't work for me: python build.py -t linux -a x64 -dkr -img ubuntu:zesty j2v8 |
|
ON OSX I was able to run a docker build for linux but it fails now due to some permission issue that I didn't have before: |
Did you build nodejs before that ? the error says that it cant find the nodejs static libraries that j2v8 wants to link to. This is because specifying only |
|
I found one culprit I think: this does not have executive rights |
|
@drywolf how am I supposed to build node, do you have helpers for this? When I only use J2V8 without NodeJS but only V8 am I still obliged to build NodeJS because it has dependencies in to V8 deps folder? You mentioned there is a way to build against nodejs version that was downloaded and already build for the platform, how does one proceed with that? The more I dig into the topic the more I see what a complicated and big fish we are trying to cook here. There are many variations... |
Yeah, IIRC we already had this issue before for the other shell scripts ... it is kind of annoying that there seems to be no way to tell git that execute permissions should be always given to .sh scripts in a repo. 😕 |
As far as I know it will always be NodeJS that is used to get a linkable set of static V8 libraries and the additional Node.js static libraries (libuv, etc.) ... please correct me if I am wrong @irbull , but I have not yet seen any indication that J2V8 was ever linked against binaries produced by building V8 from the original V8 code So in a sense if you want to link only against the V8 libs produced by a Node.js build that is fine. J2V8 is just piggybacking onto the Node.js code because from Node.js you get both options for free instead of having to support two different third party dependencies (V8 and Node.js independently).
That's the end goal ... right now you can build node.js via the
Yeah, that's also why the question posed in #261 is such a tricky one ... how to decide what is the set of binaries that this project should provide |
But you still have a important point there ... I am not sure how all the different linkers will handle this ... right now I assumed that if To make perfectly sure that no Node.js specific libs are ever linked when |
pom.xml
Outdated
| <modelVersion>4.0.0</modelVersion> | ||
| <properties> | ||
| <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding> | ||
| <os>${env.J2V8_PLATFORM_NAME}</os> | ||
| <os>alpine-linux</os> |
There was a problem hiding this comment.
is this correct? Why do we assume in pom file some alpine-linux?
There was a problem hiding this comment.
This is ok ... this string will be set to the correct value by the build-system before the java build step is running. It performs a manipulation of the XML directly, see #320 for why this should be a good change to make.
There was a problem hiding this comment.
but if this will change why this is alpine-linux by default? This is one of those things that affects the current build process right, the one which @irbull currently is running parallel to the new one (authored by you)
There was a problem hiding this comment.
Thats true, but that's also how it was before any changes I made ... there was also some current settings in the POM that were merely there because it was the last thing that was built and coincidentially I guess this was what ended up being committed to the git repo.
There was a problem hiding this comment.
I think I will put some XML comments in front of those values, then it should be clear that those are only temporary values
|
I may have already asked this question but I really don't get it. Lets say I wanna use "j2v8 alias"
This says that it requires NodeJS dependency to be prebuilt. What does it mean? Does it mean I can download it from the Internet for my platform, put into some directory and then invoke build with j2v8 alias? |
|
Is there any way we can prevent NodeJS unit tests from running? This is just another failure scenario possibility. NodeJS tests should be run within NodeJS project, they should work there not on our machines/jenkins. |
|
@drywolf I tested at least docker build with linux, debian/jessie and native osx build. Both of them work, for my needs at the moment this is LGTM but obviously there would have to be lots of test cases... maybe somebody can test android, alpine linux scenario, etc |
|
I managed to built successfully with NodeJS 7.9.0: You could add this in this PR to J2V8/node.patches folder. Sadly 7.10.x and 8.x series does not work due to: |
To clarify... But they are of no use to the J2V8 project, because we need to have the intermediate development files (object files What I am working on with the build-system / CI process is the following (just a rough outline here): 2 Roles
[J2V8 maintainer]
[J2V8 user]
|
Is this about the same thing as you mentioned above ?
That is right, and those are not just "NodeJS" tests ... those are J2V8's tests to ensure proper integration of NodeJS when it is enabled. So I can't quite agree that those are inappropriate to run in J2V8 nor do those tests have any relevance for the actual Node.js project ... forgive me if I am overlooking something here, are there any actual Node.js tests that are running when Node.js is being built ? I have not yet seen anything that would look like it ... but please let me know if you know something that I don't 😉 PS: I already have a working solution that I implemented yesterday that will not run the J2V8 NodeJS tests if the build does not have node-enabled specified. I will add it to the PR once I have time to test it some more and look into the other things that came up in your review 👍 |
|
What where the build params / the command line that you used to build ?
What do you mean ? |
|
I will try to compile again on OSX and Linux with ensure I cleaned up all stuff (I think I did). Keep in mind I compiled on OSX natively instead of vagrant... |
|
Yeah, will be interesting to see if it works from a clean build. If it does then it might have to do with some environmental difference between the Vagrant MacOSX box and the environment on your native MacOSX box. I just saw that the PS: also I didn't catch your changes to the J2V8 cpp file in your fork, I am retrying now with those changes in place. |
|
My unit tests fail on Manjaro Linux on a fresh NodeJS enabled build on linux (no docker) and 8.2.1 NodeJS As you can see it claims to be in vendor manajro mode. I guess this is minor issue because who will run j2v8 in production on Manjaro linux... but it shows that it is trickier than one thinks with LibraryLoader.java Using this command and NodeJS 8.2.1 everything builds fine: |
|
@drywolf I added --enable-static to 8.2.1 diff patch for Windows build. |
|
Thx, but with the more recent node versions the Status update from my side ... I'm now going to re-try MacOS and Linux... PS @matiwinnetou I also thought the same about communicating ... maybe we can connect via some other IM in the meantime .. do you use Skype ? |
- we must use atexit to stop Vagrant VMs / Docker containers in case of an error
|
With the tests in
By looking into android-configure and the Android NDK docs I hope it will be not too hard to get Android working also 🤞 |
|
I use Skype rarely but I can change this for this project, my SkypeId: mati22081979, please add me anybody who wishes to communicate on this project. We can create some team chat there. I would prefer gitter.im but AFAIK I cannot create Gitter chatroom as I am not a project maintainer... |
|
@matiwinnetou You can create a freenode channel, e.g. #j2v8 and it is also possible to keep a freenode channel in sync with gitter and vice versa. See for example: http://webchat.freenode.net/?channels=dat See: http://freenode.net/news/registering-a-channel-on-freenode Freenode is nice and simple and accessible to anyone. |
|
@drywolf I changed my mind, can you remove from this PR all patches to NodeJS except for 7.4.0. We should try to keep this PR only related to build system. My PR regarding NodeJS 8.2.1 upgrade will contain a new file in node.patches folder... |
| message("J2V8_LIB_PLATFORM_NAME = ${J2V8_LIB_PLATFORM_NAME}") | ||
| message("J2V8_TARGET_ARCH = ${J2V8_TARGET_ARCH}") | ||
| message("J2V8_BUILD_X64 = ${J2V8_BUILD_X64}") | ||
| message("--------------------------------------------------") |
There was a problem hiding this comment.
One thing that I am missing, that could be either here or in a 'success' message at the end, is the V8 and NodeJS versions that have been compiled into the final output (e.g. the .aar).
They are not user-selectable versions, but having them hard-coded makes it more important to report what you'll end up with after build completes.
There was a problem hiding this comment.
Good point, I will try to include this information even earlier in the build process, so a user can abort as early as possible if he meant to build something else.
I am not sure though if this information should always be displayed or if it should be only displayed when an extra command line switch is present or some kind of verbosity level is set.
I am thinking about this, because now that most features that I had planned are done, I am trying to think about the Developer-Experience and one part of that is the verbosity of the build output ... there is already a lot of stuff happening if one runs a complete build for a platform with all the build-steps involved. Two lines for the Node.js/V8 versions of course won't make the big difference there, but at least I wanted to let everyone know that I am thinking about this aspect now also.
There was a problem hiding this comment.
I would go with a message right at the start. Let's hit those devs with those 2 lines :D
That's also what I was thinking |
|
@matiwinnetou did you see my freenode suggestion in all the comments floods on this PR? I added that and Gitter usage to the future direction discussion thread. |
|
I've opened #331 for the chat request. |
- improve merging of pre-defined and user build-params for `build -i`
|
Has anyone had luck with this PR and native builds on MacOS. All the tests are failing for me with an unsatisfied link error. The build looks like it works, so I assume it's just the changes to the library loader. I'm trying to debug it now |
|
I added some debug statements to the native code, and now the build is running fine... |
|
Builds fine locally. I'm trying to get our CI builds working with this setup. Travis seems to have problems with recent versions of cmake. |
|
Do you still use the docker files in /docker? @drywolf |
|
@irbull |
|
I filed #343 to cleanup unused build files.
…On Tue, 29 Aug 2017 at 06:49 Wolfgang Steiner ***@***.***> wrote:
@irbull <https://github.com/irbull> Dockerfile.android and
Dockerfile.linux are not used by my code.
Those are among the files that I was already talking about in a previous
comment, which should be cleaned up now to make the transition complete
from the previous build scripts & files to the new system.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#327 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAMBE9YSO5JbQR0OQq48nSIW1zAR-Bduks5sdBbfgaJpZM4OrdOV>
.
|
|
congratulations everyone working on this PR, especially @drywolf 🎉 🎈 🎆 this is great stuff, great quality! i am now busy working on my non-profit movement for people who care, innercircles |
Features
win32target (should help with this requirement)alpine-linux(supercedes Support for Alpine Linux #289)--no-shutdownoption for vagrant builds (keeps the used Vagrant VM running after a build is finished)--keep-native-libsoption provides basic support for including different native libraries into a single JAR bundleall ~nodejs ~testruns all build-steps except fornodejsandtestFixes
-lrtwhen target is Linux #292, "version `GLIBCXX_3.4.15' not found" #188, Automate deployment to Maven / Gradle repositories #320