Load the native engine lib from a pkg_resource. #4914

jsirois · 2017-09-29T15:32:04Z

This switches pants to generating wheels instead of sdists and embeds
the proper platform-dependent native engine library as a resource within
the pantsbuild.pants wheel.

Fixes #4906

kwlzn

lgtm! thanks for tackling this.

kwlzn · 2017-10-06T18:55:48Z

.travis.yml

@@ -45,47 +45,40 @@ cache:
 matrix:
  include:
    - os: osx
+      # We request the oldest image we can for maximum compatibility.
+      osx_image: xcode6.4


might be good to clearly state in this comment which OSX version xcode6.4 nets us to help onlookers save time when the question inevitably comes up.

Agreed, fixed

kwlzn · 2017-10-06T21:44:52Z

src/python/pants/option/global_options.py

-    register('--native-engine-version', advanced=True,
-             default=pkg_resources.resource_string('pants.engine', 'native_engine_version').strip(),
-             help='Native engine version.')
-    register('--native-engine-supportdir', advanced=True, default='bin/native-engine',


I suppose since these are largely obscured from users in general it's fine to yank these without a deprecation cycle.

Yea, I'm fine with this.

... but it would be pretty easy to leave them in as noops with a deprecation warning?

Agreed that it's easy enough to do, fixed.

stuhood

Thanks a lot John!

stuhood · 2017-10-06T22:25:53Z

build-support/bin/native/bootstrap.sh

-# + get_rust_os_ids: Produces the BinaryUtil os id paths supported by rust, one per line.
-source ${REPO_ROOT}/build-support/bin/native/utils.sh
+# TODO(John Sirois): Eliminate this replication of BinaryUtil logic internal to pants code when
+# https://github.com/pantsbuild/pants/issues/4006 is complete.


This issue is probably no longer relevant to this codepath.

Agreed, killed. Also killed the emulation and just kept LIB_EXTENSION, which is the only variant bit needed any longer.

stuhood · 2017-10-06T22:35:45Z

build-support/bin/release.sh

@@ -490,7 +552,7 @@ function usage() {
  echo "       and can be installed in an ephemeral virtualenv."
  echo " -l  Lists all pantsbuild packages that this script releases."
  echo " -o  Lists all pantsbuild package owners."
-  echo " -e  Check that native engine binaries are deployed for this release."
+  echo " -e  Check that wheels are prebuilt for this release."


Do you think that you could add a comment somewhere to explain what process generally prebuilds the wheels (tagged travis builds? all travis builds?) and what consumes them (the release script)? Will this mean that a release tag must be pushed (and be past the relevant stage in travis) before the release can proceed? Fine either way, just hoping to understand.

Added.

Yeah, as things stand the binary wheels just get built on master pushes, in .travis.yml we still have:

on: condition: $PREPARE_DEPLOY = 1 branch: master repo: pantsbuild/pants

This means the version bump / changelog PR will (re)generate the final wheels, and the release script is checking that wheels for each published package have been curled down, so this should be failsafe mod only 1 version (mac vs linux) of the pantsbuild.pants wheel being published. That though would mean a red master CI which - for now anyway - I'm hoping we're still intolerant of.

And, I just realized this breaks down for stable branch cherry-picks. I'll update the release condition to include any https://github.com/pantsbuild/pants branch.

stuhood · 2017-10-06T22:38:10Z

build-support/docker/travis_ci/Dockerfile

+      cmake \
+      oracle-java8-installer
+
+# Setup mount points tfor the travis ci user & workdir.


s/tfor/for/

stuhood · 2017-10-06T22:40:27Z

src/python/pants/engine/native.py

-                  options.native_engine_version,
-                  options.native_engine_supportdir,
-                  options.native_engine_visualize_to)
+    return Native(options.native_engine_visualize_to)



Not a comment for you, but probably for @benjyw / @kwlzn : should we consider reverting c34fa86 after this lands? Sounds like it had some fallout anyway: #4917

no, the daemon externalization patch depends on c34fa86 and will move all of the pantsd and watchman scoped options as well as some global options into bootstrap as well. so we'll have to solve or accept #4917 very soon either way.

jsirois · 2017-10-07T01:46:34Z

Since the changes here in response to review feedback were small, I'll TBR once this goes green to move on to performing a release that tests all this jimcrackery.

This switches pants to generating wheels instead of sdists and embeds the proper platform-dependent native engine library as a resource within the pantsbuild.pants wheel. Fixes pantsbuild#4906

prebuilt wheel.

illicitonion · 2017-10-09T13:01:33Z

build-support/docker/travis_ci/Dockerfile

+VOLUME /travis/workdir
+
+# Setup a non-root user to execute the build under (avoids problems with npm install).
+ARG TRAVIS_USER=travis_ci


Why are these args which leak in from the host environment, rather than being self-contained constants in the Dockerfile?

jsirois force-pushed the jsirois/issues/4906 branch 12 times, most recently from d8e3a61 to f677213 Compare October 2, 2017 17:22

jsirois mentioned this pull request Oct 2, 2017

release pants as a pex #4896

Closed

jsirois force-pushed the jsirois/issues/4906 branch 14 times, most recently from 0138148 to a23d181 Compare October 6, 2017 17:22

jsirois requested review from illicitonion and kwlzn October 6, 2017 18:04

kwlzn approved these changes Oct 6, 2017

View reviewed changes

stuhood mentioned this pull request Oct 6, 2017

dockerize CI? #4859

Closed

stuhood approved these changes Oct 6, 2017

View reviewed changes

jsirois force-pushed the jsirois/issues/4906 branch 2 times, most recently from 111d2ff to b6fada3 Compare October 7, 2017 01:40

jsirois mentioned this pull request Oct 7, 2017

Fix BundleIntegrationTest. #4953

Merged

jsirois added 3 commits October 7, 2017 11:53

Load the native engine lib from a pkg_resource.

ffe9e4f

This switches pants to generating wheels instead of sdists and embeds the proper platform-dependent native engine library as a resource within the pantsbuild.pants wheel. Fixes pantsbuild#4906

Fixup -e / add a basic sanity check that all packages have at least one

ed8bfec

prebuilt wheel.

Fixes prompted by review feedback.

5b3cde4

jsirois force-pushed the jsirois/issues/4906 branch from b6fada3 to 5b3cde4 Compare October 7, 2017 18:54

jsirois merged commit 2a9a4b9 into pantsbuild:master Oct 7, 2017

jsirois deleted the jsirois/issues/4906 branch October 7, 2017 20:23

illicitonion reviewed Oct 9, 2017

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Load the native engine lib from a pkg_resource. #4914

Load the native engine lib from a pkg_resource. #4914

jsirois commented Sep 29, 2017

kwlzn left a comment

kwlzn Oct 6, 2017

jsirois Oct 7, 2017

kwlzn Oct 6, 2017

stuhood Oct 6, 2017 •

edited

Loading

jsirois Oct 7, 2017

stuhood left a comment

stuhood Oct 6, 2017

jsirois Oct 7, 2017

stuhood Oct 6, 2017

jsirois Oct 7, 2017

jsirois Oct 7, 2017

stuhood Oct 6, 2017

jsirois Oct 7, 2017

stuhood Oct 6, 2017

kwlzn Oct 6, 2017

jsirois commented Oct 7, 2017

illicitonion Oct 9, 2017

Load the native engine lib from a pkg_resource. #4914

Load the native engine lib from a pkg_resource. #4914

Conversation

jsirois commented Sep 29, 2017

kwlzn left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood Oct 6, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

stuhood left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

jsirois commented Oct 7, 2017

Choose a reason for hiding this comment

stuhood Oct 6, 2017 •

edited

Loading