Skip to content
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

When building a pex in release.sh, build it for all supported ABIs. #7393

Merged
merged 2 commits into from Mar 15, 2019
Merged
Changes from 1 commit
Commits
File filter...
Filter file types
Jump to…
Jump to file or symbol
Failed to load files and symbols.

Always

Just for now

@@ -569,29 +569,35 @@ function build_pex() {
# If $1 == "fetch", fetches the linux and OSX wheels which were built on travis.
local mode="$1"

local linux_platform="linux_x86_64"
local osx_platform="macosx_10.11_x86_64"
local linux_platform_noabi="linux_x86_64"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

Nit

Suggested change
local linux_platform_noabi="linux_x86_64"
local linux_platform_no_abi="linux_x86_64"
local osx_platform_noabi="macosx_10.11_x86_64"

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

Nit

Suggested change
local osx_platform_noabi="macosx_10.11_x86_64"
local osx_platform_no_abi="macosx_10.11_x86_64"

case "${mode}" in
build)
case "$(uname)" in
Darwin)
local platform="${osx_platform}"
;;
Linux)
local platform="${linux_platform}"
;;
*)
echo >&2 "Unknown uname"
exit 1
;;
# NB: When building locally, we use a platform that does not refer to the ABI version, to
# avoid needing to introspect the python ABI for this machine.
Darwin)
local platforms=("${osx_platform_noabi}")
;;
Linux)
local platforms=("${linux_platform_noabi}")
;;
*)
echo >&2 "Unknown uname"
exit 1
;;
esac
local platforms=("${platform}")
local dest="${ROOT}/dist/pants.${PANTS_UNSTABLE_VERSION}.${platform}.pex"
local stable_dest="${DEPLOY_DIR}/pex/pants.${PANTS_STABLE_VERSION}.${platform}.pex"
;;
fetch)
local platforms=("${linux_platform}" "${osx_platform}")
local platforms=()
for platform in "${linux_platform_noabi}" "${osx_platform_noabi}"; do
for abi in "cp-27-mu" "cp-27-m"; do
This conversation was marked as resolved by Eric-Arellano

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

To confirm, we only explicitly enumerate cp27-mu and cp27m, and not abi3 or anything like cp36m, because there is only an ambiguity when using Python 2 and Python 3 will always resolve correctly, right?

If this is the case, I recommend adding a comment like this:

NB: We only must enumerate cp27mu and cp27m, and not abi3 or cp36m etc., because there is only an ambiguity around ABI to use when using Python 2. Python 3 no longer has a separate ABI depending on whether the interpreter uses UCS2 vs. UCS4, so Python 3 will always correctly resolve the wheels it needs.

Also why cp-27-mu instead of cp27mu?

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

Hm looking at this again, did you test that Python 3 wheels are still picked up? If not, I think we'll want to add a separate case for Python 3 that leaves off the ABI.

It's difficult to pin Python 3's because of the introduction of abi3, which means any Python after the pinned version works. E.g. our cryptography wheel is cp34m-abi3, whereas pantsbuild.pants is cp36m-abi3, for example. Others are simply cp36m. So, having to specify every single potential ABI for Python 3 would be difficult. Instead, leaving off the ABI and solely resolving by platform should work.

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 15, 2019

Author Member

@Eric-Arellano : py3 wheels are not currently picked up, because we haven't requested them.

This comment has been minimized.

Copy link
@stuhood

stuhood Mar 15, 2019

Author Member

Also why cp-27-mu instead of cp27mu?

I tried a few things from https://github.com/pantsbuild/pex/blob/master/tests/test_platform.py and this worked.

This comment has been minimized.

Copy link
@Eric-Arellano

Eric-Arellano Mar 15, 2019

Contributor

Ah makes sense. And release.sh -p is solely for building the PEX, which cannot offer Py3 support until we close pantsbuild/pex#654.

Could you please add a comment like this:

Note we do not include Python 3 wheels, as we cannot release a Python 3 compatible PEX until https://github.com/pantsbuild/pex/issues/654.
platforms=("${platforms[@]}" "${platform}-${abi}")
done
done
local dest="${ROOT}/dist/pants.${PANTS_UNSTABLE_VERSION}.pex"
local stable_dest="${DEPLOY_DIR}/pex/pants.${PANTS_STABLE_VERSION}.pex"
;;
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.