-
-
Notifications
You must be signed in to change notification settings - Fork 632
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
Conversation
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this 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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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. |
There was a problem hiding this comment.
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 |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
lgtm
### 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.
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 like1.4.0.dev21+b9121c0c4
to1.4.0.dev21
, but probably a bit risky for heading in the opposite direction.Result
#5118 will be able to use a command like:
...to re-version whls fetched from travis.
I did some local testing to confirm that pip is able to install a re-versioned whl.