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

Utility to reversion whls #5145

Merged
merged 7 commits into from
Nov 30, 2017
Merged

Conversation

stuhood
Copy link
Sponsor Member

@stuhood stuhood commented Nov 30, 2017

Problem

After feedback in #5118, #4896 will now propose to build suffixed releases of pants in travis on relevant platforms, fetch them them to a releaser's machine, and then "stabilize"/"re-version" them with an unsuffixed version before sending them to pypi.

AFAICT, there are no utilities for re-versioning wheels around (perhaps because it's not a great idea? *shrug).

Solution

Implement a tool for re-versioning whl files by find-replacing the version str in all files in the whl and then updating the RECORD file with new fingerprints. Because of the blind find-replace, this is useful for stabilizing from a version like 1.4.0.dev21+b9121c0c4 to 1.4.0.dev21, but probably a bit risky for heading in the opposite direction.

Result

#5118 will be able to use a command like:

./pants -q run src/python/pants/releases:reversion -- requests-2.18.4-py2.py3-none-any.whl ${dist} 17.4.0.dev21

...to re-version whls fetched from travis.

I did some local testing to confirm that pip is able to install a re-versioned whl.

@stuhood stuhood mentioned this pull request Nov 30, 2017
with temporary_dir() as dest_dir:
# Download an input whl.
# TODO: Not happy about downloading things. Attempted to:
# ./pants setup-py bdist_wheel -w $dest_dir
Copy link
Sponsor Member Author

Choose a reason for hiding this comment

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

Will fix before landing, but this was more like:

./pants setup-py --run="bdist_wheel" examples/src/thrift/org/pantsbuild/example/distance:distance-python

...which, when executed directly, results in a whl at:

dist/pantsbuild.pants.distance-thrift-python-0.0.1/dist/pantsbuild.pants.distance_thrift_python-0.0.1-py2-none-any.whl

When executed in the context of a test, the command completes successfully with the same stdout, but with no whl created.

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.

LGTM mod the test. One suggestion for making this less scary below.

def main():
"""Given an input whl file and target version, create a copy of the whl with that version.

This is accomplished via a ton of string replacement: if the input version is not sufficiently
Copy link
Contributor

@jsirois jsirois Nov 30, 2017

Choose a reason for hiding this comment

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

IIUC there is exactly 1 wheel we need to do a find-replace on non-wheel metadata files ("content files"), that for the pantsbuild.pants wheel itself. Pants plugin wheels won't have a this version embedded in "content files" and all other dependencies won't be re-versioned at all. If this is correct, we know the 1 file to edit in that case. Would it be reasonable to just have the tool take an optional list of positional arguments that correspond to wheel "content files" to edit or even positional args forming a command line to execute in the chroot of the exploded wheel for extra transformations needed?

All this said, I think this is probably fine as-is.

Copy link
Member

Choose a reason for hiding this comment

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

+1 to more specific file targeting (maybe a glob match?) - I could easily see replacing these strings against all files in the wheel having unintended side effects.

]
self.assert_success(self.run_pants(command))

# TODO: confirm usable.
Copy link
Member

Choose a reason for hiding this comment

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

completing this TODO seems pretty critical to the correctness of this patch - may be worth delaying landing for? could either apply the wheel API directly or maybe depend on and invoke pex as a CLI inline?

def main():
"""Given an input whl file and target version, create a copy of the whl with that version.

This is accomplished via a ton of string replacement: if the input version is not sufficiently
Copy link
Member

Choose a reason for hiding this comment

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

+1 to more specific file targeting (maybe a glob match?) - I could easily see replacing these strings against all files in the wheel having unintended side effects.

Copy link
Member

@kwlzn kwlzn left a comment

Choose a reason for hiding this comment

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

lgtm

@stuhood stuhood merged commit ef5c12b into pantsbuild:master Nov 30, 2017
@stuhood stuhood deleted the stuhood/reversion branch November 30, 2017 22:38
stuhood pushed a commit that referenced this pull request Dec 1, 2017
### Problem

As described on #4896, the "binary builder" shards in travis currently overwrite the previous artifacts for a SHA, meaning that it is possible for `release.sh` to race with additional commits landing in master.

Relatedly, because we build artifacts for all shas and not just tags (and should continue to do so in order to solve #4896), it's not possible to differentiate the `whl`s that were built for an actual release from those built for any old dev sha. 

### Solution

1. Build version-suffixed `whl`s for all travis builds.
    * This will allow consumers to build a pex for any SHA that has been built in travis unambiguously, while avoiding cache collisions due to re-used version numbers.
2. SHA-prefix the artifacts built by the "binary builder" shards to avoid problematic collisions between published artifacts.
3. "Re-version" whls that we release to PyPI (via #5145).

### Result

This change makes releases less racy, and begins to unblock building pexes from any SHA with #4896 in future PRs.
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.

3 participants