Skip to content
This repository has been archived by the owner on Mar 8, 2020. It is now read-only.

Fix build for macOS #124

Merged
merged 10 commits into from
Aug 28, 2019
Merged

Fix build for macOS #124

merged 10 commits into from
Aug 28, 2019

Conversation

ncordon
Copy link
Member

@ncordon ncordon commented Aug 27, 2019

  • Adds a job in travis to check release setting, i.e. would the project cross compile correctly in the release stage?
  • Solves cross compilation issues for MacOS. Right now there is trouble cross compiling the project for sdk version 10.11.

Closes #123

Signed-off-by: ncordon nacho.cordon.castillo@gmail.com


This change is Reviewable

@ncordon ncordon force-pushed the travis.enhancements branch 4 times, most recently from 3bdbc29 to 4f54a37 Compare August 27, 2019 16:16
@bzz
Copy link
Contributor

bzz commented Aug 27, 2019

Nice, we have a CI failure on the new profile 💃

@bzz
Copy link
Contributor

bzz commented Aug 27, 2019

bollocks, we forgot - cd $TRAVIS_BUILD_DIR in the script section! 🤦‍♂

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
.travis.yml Outdated Show resolved Hide resolved
Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@bzz
Copy link
Contributor

bzz commented Aug 27, 2019

Locally on linux, does the

        - curl -sSL "https://codeload.github.com/tpoechtrager/osxcross/tar.gz/${OSXCROSS_REV}" | tar -C "${PWD}" --strip=1 -xzf -
        - curl -sSL -o tarballs/MacOSX${SDK_VERSION}.sdk.tar.xz ${OSXCROSS_SDK_URL}
        - UNATTENDED=yes ./build.sh >/dev/null

work for you, with the the new archive?

@creachadair creachadair changed the title Fix build for MacOs Fix build for macOS Aug 27, 2019
Older one did not include support for sdk 10.13

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@ncordon
Copy link
Member Author

ncordon commented Aug 27, 2019

Locally on linux, does the

        - curl -sSL "https://codeload.github.com/tpoechtrager/osxcross/tar.gz/${OSXCROSS_REV}" | tar -C "${PWD}" --strip=1 -xzf -
        - curl -sSL -o tarballs/MacOSX${SDK_VERSION}.sdk.tar.xz ${OSXCROSS_SDK_URL}
        - UNATTENDED=yes ./build.sh >/dev/null

work for you, with the the new archive?

What was not working for me was the line

curl -sSL -o tarballs/MacOSX${SDK_VERSION}.sdk.tar.xz ${OSXCROSS_SDK_URL}

but because of permissions of curl. In travis it works. Locally what did work for me was (and maybe it makes sense to do it on the travis also):

wget ${OSXCROSS_SDK_URL} -O tarballs/MacOSX${SDK_VERSION}.sdk.tar.xz

The other two lines do work for me on local

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@ncordon ncordon force-pushed the travis.enhancements branch 4 times, most recently from 8163b14 to 8c63eba Compare August 28, 2019 10:55
New versions of osxcross include a patch for _hash_table, which probably solves our problem

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019

I know this is ultra hacky. I have documented a proposal for improving this (and why it has to be so hacky right now) in this issue #87

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

I have a few minor mostly documentation-related comments, but nothing blocking.

Reviewed 2 of 2 files at r4.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dennwc, @kuba--, and @ncordon)


.travis.yml, line 23 at r4 (raw file):

env:
  global:
   - OSXCROSS_PATCH_REV=f67ce5033d0a01292c9ef156b95c025a3e1edcbc

Maybe this should be done as a separate change, but it would be nice to have some documentation about what these values are for. Particularly in this case as we are adding a separate patch explicitly during the build process: When someone wants to update the base version of osxcross, for example, how can they safely do that without disrupting the patch? (Or: What do they have to do to update the patch at the same time? Or: Can we get rid of the patch once a certain release hits? Or whatever is true here).


.travis.yml, line 65 at r4 (raw file):

        - curl -sSL "https://github.com/karalabe/xgo/blob/${XGO_REV}/docker/base/patch.tar.xz?raw=true" | xz -dc - | tar -xf -
        - mv v1 "target/SDK/MacOSX${SDK_VERSION}.sdk/usr/include/c++/v1"
        # Fixes __hash_table file with a patch included in osxcross

Will we eventually be able to get rid of this? Can you say something about what conditions would allow us to not need this anymore? If possible is there an issue link in the underlying project?


build.sbt, line 77 at r4 (raw file):

val SONATYPE_PASSPHRASE = scala.util.Properties.envOrElse("SONATYPE_PASSPHRASE", "not set")
val JAVA_HOME = scala.util.Properties.envOrElse("JAVA_HOME", "/usr/lib/jvm/java-8-openjdk-amd64")
val CPP_FLAGS = " -shared -Wall -fPIC -O2 -std=c++11 "

I suggest not requiring extra whitespace around these values, but insert spaces when composing the command line as needed. Or if that is too big a pain, I think we should at least document that those spaces are needed so that future edits don't clobber them. (Also that quoting is required around internal whitespace, though in the arguments we have here there aren't any)


build.sbt, line 78 at r4 (raw file):

val JAVA_HOME = scala.util.Properties.envOrElse("JAVA_HOME", "/usr/lib/jvm/java-8-openjdk-amd64")
val CPP_FLAGS = " -shared -Wall -fPIC -O2 -std=c++11 "
val GCC_FLAGS = " -Wl,-Bsymbolic "

Can these be val const?

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @dennwc, @kuba--, and @ncordon)

@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019


build.sbt, line 78 at r4 (raw file):

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

Can these be val const?

vals are immutable in Scala (vars are the ones that act as modifiable variables). It is a normal confusion though :)

@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019


.travis.yml, line 23 at r4 (raw file):

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

Maybe this should be done as a separate change, but it would be nice to have some documentation about what these values are for. Particularly in this case as we are adding a separate patch explicitly during the build process: When someone wants to update the base version of osxcross, for example, how can they safely do that without disrupting the patch? (Or: What do they have to do to update the patch at the same time? Or: Can we get rid of the patch once a certain release hits? Or whatever is true here).

I will add a comment explaining it: both OSXCROSS_PATCH_REV and OSXCROSS_REV are the revisions of osxcross repo. The OSXCROSS_REV was already used to retrieve a version of that tool that did not include the .patch file we need. I tried updating the former one too (because OSXCROSS_PATCH_REV is newer), but everything broke again, both locally and in Travis. To update osxcross one should update OSXCROSS_REV, the other one is just used to download a very specific .patch file. I would not recommend doing it for now though, because the .patch is applied by lines, and if the versions of the stdlib the in the repo changed, we would not be able to patch them. All of this boilerplate should go away if as I discussed here we drop support for macOs 10.6 and bump to 10.7 everywhere, but I want feedback from mac users from the team / company.

@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019


.travis.yml, line 65 at r4 (raw file):

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

Will we eventually be able to get rid of this? Can you say something about what conditions would allow us to not need this anymore? If possible is there an issue link in the underlying project?

Yes, use OSX_VERSION_MIN=10.7 instead of OSX_VERSION_MIN=10.6 should solve the problem and delete a lot of the hacky things we have here

@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019


build.sbt, line 77 at r4 (raw file):

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

I suggest not requiring extra whitespace around these values, but insert spaces when composing the command line as needed. Or if that is too big a pain, I think we should at least document that those spaces are needed so that future edits don't clobber them. (Also that quoting is required around internal whitespace, though in the arguments we have here there aren't any)

Will change it : )

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

Reviewable status: all files reviewed, 3 unresolved discussions (waiting on @creachadair, @dennwc, @kuba--, and @ncordon)


.travis.yml, line 65 at r4 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Yes, use OSX_VERSION_MIN=10.7 instead of OSX_VERSION_MIN=10.6 should solve the problem and delete a lot of the hacky things we have here

Is there any compelling reason for us not to use 10.7 instead of 10.6? Snow Leopard was released in 2009, I think people have had long enough to upgrade. 😁

I'm fine with doing this for now, though, to keep things working if you think that should be a separate question.


build.sbt, line 78 at r4 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

vals are immutable in Scala (vars are the ones that act as modifiable variables). It is a normal confusion though :)

🤦‍♀️ This is not even the first time I've made that mistake.

Signed-off-by: ncordon <nacho.cordon.castillo@gmail.com>
@ncordon
Copy link
Member Author

ncordon commented Aug 28, 2019


.travis.yml, line 65 at r4 (raw file):

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

Is there any compelling reason for us not to use 10.7 instead of 10.6? Snow Leopard was released in 2009, I think people have had long enough to upgrade. 😁

I'm fine with doing this for now, though, to keep things working if you think that should be a separate question.

Not to the best of my knowledge, I would root for upgrading to that version

Copy link
Contributor

@creachadair creachadair left a comment

Choose a reason for hiding this comment

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

Reviewed 2 of 2 files at r5.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @dennwc, @kuba--, and @ncordon)


.travis.yml, line 23 at r4 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

I will add a comment explaining it: both OSXCROSS_PATCH_REV and OSXCROSS_REV are the revisions of osxcross repo. The OSXCROSS_REV was already used to retrieve a version of that tool that did not include the .patch file we need. I tried updating the former one too (because OSXCROSS_PATCH_REV is newer), but everything broke again, both locally and in Travis. To update osxcross one should update OSXCROSS_REV, the other one is just used to download a very specific .patch file. I would not recommend doing it for now though, because the .patch is applied by lines, and if the versions of the stdlib the in the repo changed, we would not be able to patch them. All of this boilerplate should go away if as I discussed here we drop support for macOs 10.6 and bump to 10.7 everywhere, but I want feedback from mac users from the team / company.

Thank you!


.travis.yml, line 65 at r4 (raw file):

Previously, ncordon (Nacho Cordón) wrote…

Not to the best of my knowledge, I would root for upgrading to that version

👍

@ncordon ncordon merged commit fdc54a8 into bblfsh:master Aug 28, 2019
@ncordon ncordon deleted the travis.enhancements branch August 28, 2019 17:07
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Publish latest release on Maven
3 participants