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

Deprecate python_binary in favor of pex_binary #10939

Merged
merged 3 commits into from
Oct 11, 2020

Conversation

Eric-Arellano
Copy link
Contributor

Reasons:

  1. Users have expressed interest in other formats than Pex, such as Pyinstaller. We don't want to privilege Pex over those other formats.
  2. More explicit to users what it is Pants is doing. python_binary is not some magical thing; it is an interface around the Pex tool.
    • Reminder: we are framing Pants as "an orchestrator around the tools you already use". A theme of 2.0 has been to reduce places where Pants is magical, e.g. our revised __init__.py handling.

We want to make this change before 2.0 is released to the world.

[ci skip-rust]
[ci skip-build-wheels]

[ci skip-rust]
[ci skip-build-wheels]
Comment on lines 296 to 297
f"Use the target type `pex_binary`, rather than `python_binary` for {address}. "
"The behavior is identical."
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I originally wanted to add a sed snippet, but gave up. Sed has some differences between macOS and Linux that I personally find hard to navigate.

The snippet is easy with rg and sd (modern Rust alternative), but too few users have those installed.

Distributing a Python script felt super overkill.

--

Lmk if we should in fact add some snippet, or at least the regex ^python_binary\(.

Copy link
Contributor

@jsirois jsirois Oct 9, 2020

Choose a reason for hiding this comment

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

We use sed -Ee "..." for mac and linux in out own scripts successfully and stably. Did you need some sed feature beyond -E extended regexps?:

$ git grep -w sed *.sh
build-support/bin/contributors.sh:    | sed -e 's/;.*$//'
build-support/bin/contributors.sh:    sed -E -e "s|^|+ |" >> CONTRIBUTORS.md
build-support/bin/release-changelog-helper.sh:        sed -Ee "s|^\(#([0-9]+)\)$|https://github.com/pantsbuild/pants/pull/\1|"
build-support/bin/release-changelog-helper.sh:        sed -Ee "s|^.*(https?://[^ ]+).*$|\1|" | \
build-support/bin/release-changelog-helper.sh:        sed -Ee "s|[/\.]+$||"
build-support/bin/release.sh:    PEX_VERSION="$(requirement pex | sed -e "s|pex==||")"

Copy link
Contributor

@cosmicexplorer cosmicexplorer Oct 11, 2020

Choose a reason for hiding this comment

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

sed -Ee 's#^python_binary\(#pex_binary(#g' would work I think? I wonder if #9434 would become more tolerable now since we already have libcst as a dependency? It would be able to act on BUILD files that aren't perfectly formatted is all (where ^python_binary() may not necessarily catch all of them).

Copy link
Contributor

Choose a reason for hiding this comment

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

A suggestion for after the 2.0 release is I believe how to appropriately consider #9434.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you John and Danny for the help!

Copy link
Contributor

@jsirois jsirois left a comment

Choose a reason for hiding this comment

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

As to reasons: there is only one - python_binary has pex specific fields ... so it should be pex specific. This reasoning is generic and can be applied consistently to all our target types.

# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
# Rust tests and lints will be skipped. Delete if not intended.
[ci skip-rust]

# Building wheels and fs_util will be skipped. Delete if not intended.
[ci skip-build-wheels]
@coveralls
Copy link

Coverage Status

Coverage remained the same at 0.0% when pulling 020b30c on Eric-Arellano:pex-binary into f678abf on pantsbuild:master.

@Eric-Arellano Eric-Arellano merged commit 219cd42 into pantsbuild:master Oct 11, 2020
@Eric-Arellano Eric-Arellano deleted the pex-binary branch October 11, 2020 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants