Refactor CI

[ncordon]

• Builds for OSX in that operating system instead of cross compile. This cleans the pair of hacks we had in the .travis.yml script.
• Adds support for Windows, adding a new job in Travis that builds the dll
• Pushes static libraries generated for Linux, OSX and Windows to GH releases
• Creates new job that gathers those 3 libraries and builds the scala-client with them embedded. Downside: that increases the size of the package.
• Simplifies some of the build process using a makefile bash script to split fetching of dependencies from build of the project. README updated with new way of fetching dependencies with the script and compiling the project.

The process in Travis would look something alike:

Build and test in Linux ----> [if tag] Push .so to GH releases  ----------|
                                                                          |
Build in OSX ---------------> [if tag] Push .dylib to GH releases  --------------> [if tag] Download so,dylib,dll,    
                                                                          |                 build, push to Maven
Build in Windows -----------> [if tag] Push .dll to GH releases ----------|

Closes #87, closes #88, closes #102

---

This change is 

[ncordon]

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 844 at r1 (raw file):

Java_org_bblfsh_client_v2_Context_00024_create(JNIEnv *env, jobject self) {
  Context *c = new Context();
  return (jlong) c;

I am concerned about this. I mean, there is no change in this PR really, I am just making sure to cast the pointer to jlong and not just long, which is what we did before. But jlong in my Linux is a long (note this is what we had before), and in the Windows setting I have tested, a long long. What would happen if Context* c is assigned a long long (64 bits) address in memory? Are we casting it to a long (32 bits) in the case of Linux? That probably would result in a SIGSEV if we try to access a wrong address

[ncordon]

---

build.sh, line 132 at r1 (raw file):

}

for arg in "$@"; do

Suggestions about better ways of doing the argument parsing here? I have tested getopt command, but did not managed to make it work correctly

[ncordon]

---

build.sh, line 132 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Suggestions about better ways of doing the argument parsing here? I have tested getopt command, but did not managed to make it work correctly

Desirable thing is I have an autocompletion pressing TAB for the commands available in the script (do not know if that can be done), and an order of arguments that does not matter (it should only matter the fed commands, not the order we feed them to the script)

[ncordon]

@bblfsh/maintainers ready for review 😄

[creachadair]

Reviewed 4 of 7 files at r1, 3 of 3 files at r3.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on @ncordon)

---

.travis.yml, line 71 at r3 (raw file):

      language: cpp
      before_script:
        - choco install adoptopenjdk8 --version=8.222

Is choco part of the base image, or do we need to install it?

Separately: Can you please add a comment about why this particular version (and/or when it is safe to switch it)? It doesn't need to be exhaustively detailed, just a breadcrumb for someone doing updates later.

---

.travis.yml, line 81 at r3 (raw file):

      stage: release
      before_script:
        - ASSETS_URL="https://github.com/bblfsh/scala-client/releases/download/$TRAVIS_TAG"

Do these scripts run with set -e?

---

build.sh, line 132 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Desirable thing is I have an autocompletion pressing TAB for the commands available in the script (do not know if that can be done), and an order of arguments that does not matter (it should only matter the fed commands, not the order we feed them to the script)

POSIX getopt and Bash getopts do not support "long" option syntax; the GNU CLI version does, but I'd rather not pull in yet another dependency. Given that these are just sequential and none of them require arguments, this is probably fine.

I would suggest adding a --help, though, and maybe also print usage if no arguments are given, since I think most people expect a script without arguments to either do something or say why it couldn't.

---

build.sh, line 2 at r3 (raw file):

#!/bin/bash

Please add a comment below the shebang line, to summarize what this script does.

My main high-level comment is that this script isn't consistent about error-checking. Some of the functions do use &&, but not all—and if something goes wrong with (say) fetching the SDK the world will be in an unknown state. Consider maybe adding set -e so that execution errors will terminate the script early rather than cascading. That will also let you drop all the && cascades and maybe that will be easier to read.

---

build.sh, line 49 at r3 (raw file):

    echo "[clean] Cleaning downloads and compilation files..."
    rm -rf src/main/resources/{lib,libuast}
    rm -rf ${UNZIP_DIR}

Please quote and terminate arguments: `rm -rf -- "${UNZIP_DIR}"

---

build.sh, line 68 at r3 (raw file):

    rm -rf ${UNZIP_DIR} ${TAR_FILE}

    echo "[sdk] Done unpacking SDK"

Maybe put the diagnostics to stderr?

echo "[sdk] Done unpacking SDK" 1>&2

---

build.sh, line 73 at r3 (raw file):

# Checks that the library has been downloaded correctly
function checkLibuastDownload {
    find src/main/resources

I'm not sure why this is here? Is this just for diagnostic purposes?

(If so: Maybe redirect to stderr? Possibly also filter plain files: find src/main/resources -type f 1>&2)

---

build.sh, line 149 at r3 (raw file):

            ;;
        *)
            echo "Wrong argument: $arg"

(minor nit) probably "invalid" or "unsupported" rather than "wrong"

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 844 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

I am concerned about this. I mean, there is no change in this PR really, I am just making sure to cast the pointer to jlong and not just long, which is what we did before. But jlong in my Linux is a long (note this is what we had before), and in the Windows setting I have tested, a long long. What would happen if Context* c is assigned a long long (64 bits) address in memory? Are we casting it to a long (32 bits) in the case of Linux? That probably would result in a SIGSEV if we try to access a wrong address

Since we're switching from one naked cast to another, I don't think we have made anything worse. We might get better diagnostics during compilation if we use static_cast<jlong>(c) instead, though. It's always dangerous to cast between arbitrary integer types and pointers, though, unless we are using a type like uintptr_t whose API is to have enough bits for the pointer without loss.

[ncordon]

---

.travis.yml, line 71 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Is choco part of the base image, or do we need to install it?

Separately: Can you please add a comment about why this particular version (and/or when it is safe to switch it)? It doesn't need to be exhaustively detailed, just a breadcrumb for someone doing updates later.

choco is part of the Windows image Travis provides. The version is just for convenience. We need JDK 8, but since we need to export the JAVA_PATH after that we need to choose a revision of the v8 (the path depends on it). It may be safe to update as long as someone gets the JAVA_PATH updated correctly also.

[ncordon]

---

.travis.yml, line 71 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

choco is part of the Windows image Travis provides. The version is just for convenience. We need JDK 8, but since we need to export the JAVA_PATH after that we need to choose a revision of the v8 (the path depends on it). It may be safe to update as long as someone gets the JAVA_PATH updated correctly also.

Will add the comment of course

[ncordon]

---

.travis.yml, line 81 at r3 (raw file):

set -e

Nope, but I should add it probably, shouldn't I?

[ncordon]

---

build.sh, line 2 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Please add a comment below the shebang line, to summarize what this script does.

My main high-level comment is that this script isn't consistent about error-checking. Some of the functions do use &&, but not all—and if something goes wrong with (say) fetching the SDK the world will be in an unknown state. Consider maybe adding set -e so that execution errors will terminate the script early rather than cascading. That will also let you drop all the && cascades and maybe that will be easier to read.

If I add set -e at the beginning is enough? Or do I have to add it at the beginning of each function?

[ncordon]

---

build.sh, line 49 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Please quote and terminate arguments: `rm -rf -- "${UNZIP_DIR}"

Will do

[ncordon]

---

build.sh, line 68 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Maybe put the diagnostics to stderr?

echo "[sdk] Done unpacking SDK" 1>&2

👌

[ncordon]

---

build.sh, line 73 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

I'm not sure why this is here? Is this just for diagnostic purposes?

(If so: Maybe redirect to stderr? Possibly also filter plain files: find src/main/resources -type f 1>&2)

This was copied from the Scala build script. It just lists the folder where the library should have been generated. I tried to change it for a ls -R /src/main/resources but the output of the find command is much clearer.

[ncordon]

---

build.sh, line 132 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

POSIX getopt and Bash getopts do not support "long" option syntax; the GNU CLI version does, but I'd rather not pull in yet another dependency. Given that these are just sequential and none of them require arguments, this is probably fine.

I would suggest adding a --help, though, and maybe also print usage if no arguments are given, since I think most people expect a script without arguments to either do something or say why it couldn't.

Yes, those are good suggestions :). Will do

[ncordon]

---

build.sh, line 149 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

(minor nit) probably "invalid" or "unsupported" rather than "wrong"

👌

[ncordon]

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 844 at r1 (raw file):

static_cast(c)

static_cast cannot convert from ‘{anonymous}::Context*’ to ‘jlong’ {aka ‘long int’}

The only material I have found about this is in Google's JNI tips which says:

To support architectures that use 64-bit pointers, use a long field rather than an int when storing a pointer to a native structure in a Java field.

which clearly is not working for 64 bits (compiler complains). I have also found this blog post saying that doing jlong(c) (which is equivalent to what we have done should work, whereas (long) c does not). But I would like some sort of official docs confirmation

[ncordon]

I can confirm this seems to work both from a Git Bash and a Powershell in Windows

[ncordon]

Friendly ping @creachadair. All the comments have been addressed (I think)

[creachadair]

Reviewed 3 of 3 files at r4.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on @ncordon)

---

.travis.yml, line 81 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

set -e

Nope, but I should add it probably, shouldn't I?

As a rule of thumb I prefer to do it for setup scripts, where otherwise ignoring an an early failure can make it difficult to debug an indirect error later. I wouldn't bother for diagnostic or cleanup scripts, where a failure doesn't affect the correctness of the results.

---

build.sh, line 2 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

If I add set -e at the beginning is enough? Or do I have to add it at the beginning of each function?

You don't need to add it to each function, setting it at the top level affects the whole shell (though I think it does not propagate to subshells).

---

build.sh, line 73 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

This was copied from the Scala build script. It just lists the folder where the library should have been generated. I tried to change it for a ls -R /src/main/resources but the output of the find command is much clearer.

Having it as find is fine by me, I just wasn't sure why. It sounds like it's just for diagnostics, in which case I'd suggest including the redirect to stderr so that all the log output stays together.

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 844 at r1 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

static_cast(c)

static_cast cannot convert from ‘{anonymous}::Context*’ to ‘jlong’ {aka ‘long int’}

The only material I have found about this is in Google's JNI tips which says:

To support architectures that use 64-bit pointers, use a long field rather than an int when storing a pointer to a native structure in a Java field.

which clearly is not working for 64 bits (compiler complains). I have also found this blog post saying that doing jlong(c) (which is equivalent to what we have done should work, whereas (long) c does not). But I would like some sort of official docs confirmation

Ah, I overlooked that we are starting from a pointer, so what we would really want in this case is reinterpret_cast not static_cast. But that's basically the same effect apart from notation.

[ncordon]

---

build.sh, line 2 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

You don't need to add it to each function, setting it at the top level affects the whole shell (though I think it does not propagate to subshells).

Are you sure? I read this in man sed:

         Application writers should avoid relying on set −e within functions. For example, in the following script:

           set -e
           start() {
               some_server
               echo some_server started successfully
           }
           start || echo >&2 some_server failed

       the −e setting is ignored within the function body (because the function is a  command  in  an  AND-OR  list
       other  than the last). Therefore, if some_server fails, the function carries on to echo "some_serverstarted‐
       successfully", and the exit status of the function is zero (which means "some_serverfailed" is not output).

[ncordon]

---

build.sh, line 73 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Having it as find is fine by me, I just wasn't sure why. It sounds like it's just for diagnostics, in which case I'd suggest including the redirect to stderr so that all the log output stays together.

Done!

[creachadair]

Reviewable status: 6 of 7 files reviewed, 5 unresolved discussions (waiting on @creachadair and @ncordon)

---

build.sh, line 2 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Are you sure? I read this in man sed:

         Application writers should avoid relying on set −e within functions. For example, in the following script:

           set -e
           start() {
               some_server
               echo some_server started successfully
           }
           start || echo >&2 some_server failed

       the −e setting is ignored within the function body (because the function is a  command  in  an  AND-OR  list
       other  than the last). Therefore, if some_server fails, the function carries on to echo "some_serverstarted‐
       successfully", and the exit status of the function is zero (which means "some_serverfailed" is not output).

That advice is good, because awk doesn't know what shell it's using, and POSIX does not specify IIRC. But here we explicitly specify Bash, which I think does the right thing at least in versions we are likely to see anywhere in the wild.

I believe it is harmless to add it explicitly in each function, however, at least in Bash. I certainly don't object to doing both if you want to play it safe.

[ncordon]

---

.travis.yml, line 81 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

As a rule of thumb I prefer to do it for setup scripts, where otherwise ignoring an an early failure can make it difficult to debug an indirect error later. I wouldn't bother for diagnostic or cleanup scripts, where a failure doesn't affect the correctness of the results.

Problem with the before_script section I think is that even if we get an error there I think the script section would execute anyway. Should I merge before_script and script?

[ncordon]

---

build.sh, line 2 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

That advice is good, because awk doesn't know what shell it's using, and POSIX does not specify IIRC. But here we explicitly specify Bash, which I think does the right thing at least in versions we are likely to see anywhere in the wild.

I believe it is harmless to add it explicitly in each function, however, at least in Bash. I certainly don't object to doing both if you want to play it safe.

Added to the header of the file also!

[ncordon]

---

src/main/native/org_bblfsh_client_v2_libuast_Libuast.cc, line 844 at r1 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Ah, I overlooked that we are starting from a pointer, so what we would really want in this case is reinterpret_cast not static_cast. But that's basically the same effect apart from notation.

Changed to reinterpret_cast now

[creachadair]

Reviewed 4 of 4 files at r5.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @ncordon)

---

.travis.yml, line 81 at r3 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Problem with the before_script section I think is that even if we get an error there I think the script section would execute anyway. Should I merge before_script and script?

Hmm. If indeed an error in before_script doesn't prevent script from running, that would probably make sense. It seems like if we don't succeed in fetching these various assets, there's no way the rest of the build can succeed. (Or if it could: Maybe we can just delete these assets entirely? 😀)

[ncordon]

---

.travis.yml, line 81 at r3 (raw file):

Previously, creachadair (M. J. Fromberger) wrote…

Hmm. If indeed an error in before_script doesn't prevent script from running, that would probably make sense. It seems like if we don't succeed in fetching these various assets, there's no way the rest of the build can succeed. (Or if it could: Maybe we can just delete these assets entirely? 😀)

I have moved the download of those assets to script :)

[creachadair]

Reviewed 1 of 1 files at r6.
Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @ncordon)