Fix direct_url in python PEP660 editable wheels #20486
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
We were using the first entry in `build_time_source_roots` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `build_time_source_roots` gets sorted before creating the `DistBuildRequest`, so the first entry is not actually the `source_root` of the `python_distribution` target. To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel.
cognifloyd
added
needs-cherrypick
category:bugfix
benjyw
approved these changes
Feb 5, 2024
WorkerPants
pushed a commit
that referenced
this pull request
Feb 6, 2024
We were using the first entry in `build_time_source_roots` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `build_time_source_roots` gets sorted before creating the `DistBuildRequest`, so the first entry is not actually the `source_root` of the `python_distribution` target. To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel. Functionally, the editable wheels get installed just fine. However, the output of `pip list` in an exported venv might show the wrong path if there happens to be a source_root that sorted before the source_root of the distribution. Importing the python code still works, however, as all of the source_roots are included in the `.pth` file. Thus, this is a cosmetic / correctness bug.
|
I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants. ✔️ 2.19.xSuccessfully opened #20494. Thanks again for your contributions! |
cognifloyd
added a commit
that referenced
this pull request
Feb 6, 2024
… (#20494) We were using the first entry in `build_time_source_roots` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `build_time_source_roots` gets sorted before creating the `DistBuildRequest`, so the first entry is not actually the `source_root` of the `python_distribution` target. To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel. Functionally, the editable wheels get installed just fine. However, the output of `pip list` in an exported venv might show the wrong path if there happens to be a source_root that sorted before the source_root of the distribution. Importing the python code still works, however, as all of the source_roots are included in the `.pth` file. Thus, this is a cosmetic / correctness bug. Co-authored-by: Jacob Floyd <[email protected]>
WorkerPants
pushed a commit
that referenced
this pull request
Feb 24, 2024
We were using the first entry in `build_time_source_roots` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `build_time_source_roots` gets sorted before creating the `DistBuildRequest`, so the first entry is not actually the `source_root` of the `python_distribution` target. To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel. Functionally, the editable wheels get installed just fine. However, the output of `pip list` in an exported venv might show the wrong path if there happens to be a source_root that sorted before the source_root of the distribution. Importing the python code still works, however, as all of the source_roots are included in the `.pth` file. Thus, this is a cosmetic / correctness bug.
|
I tried to automatically cherry-pick this change back to each relevant milestone, so that it is available in those older releases of Pants. ✔️ 2.18.xSuccessfully opened #20602. ❌ 2.19.xI was unable to cherry-pick this PR to 2.19.x, likely due to merge-conflicts. Steps to Cherry-Pick locallyTo resolve:
Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary. ❌ 2.20.xI was unable to cherry-pick this PR to 2.20.x, likely due to merge-conflicts. Steps to Cherry-Pick locallyTo resolve:
Please note that I cannot re-run CI if a job fails. Please work with your PR approver(s) to re-run CI if necessary. When you're done manually cherry-picking, please remove the Thanks again for your contributions! |
cognifloyd
added a commit
that referenced
this pull request
Feb 24, 2024
… (#20602) We were using the first entry in `build_time_source_roots` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `build_time_source_roots` gets sorted before creating the `DistBuildRequest`, so the first entry is not actually the `source_root` of the `python_distribution` target. To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel. Functionally, the editable wheels get installed just fine. However, the output of `pip list` in an exported venv might show the wrong path if there happens to be a source_root that sorted before the source_root of the distribution. Importing the python code still works, however, as all of the source_roots are included in the `.pth` file. Thus, this is a cosmetic / correctness bug. Co-authored-by: Jacob Floyd <[email protected]>
|
Dang. This was not a complete fix for this issue. I'll have to push a follow-up PR to finish fixing it. |
cognifloyd
added a commit
that referenced
this pull request
Apr 16, 2024
We were using the first entry in `source_roots_result.path_to_root` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `SourceRootsRequest.dirs` and `SourceRootsResult.path_to_root` both get sorted as soon as the dataclass is created (if not before). PR #20486 was my first attempt at fixing this, but that wasn't enough. Since the heuristics used before #20486 and in #20486 were both inaccurate due to dataclasses getting sorted, this abandons the heuristics altogether, explicitly requesting the source root for the `python_distribution` target.
cognifloyd
added a commit
that referenced
this pull request
Apr 18, 2024
We were using the first entry in `source_roots_result.path_to_root` to populate the editable wheel's `direct_url`. However, that does not work as I expected it to because `SourceRootsRequest.dirs` and `SourceRootsResult.path_to_root` both get sorted as soon as the dataclass is created (if not before). PR #20486 was my first attempt at fixing this, but that wasn't enough. Since the heuristics used before #20486 and in #20486 were both inaccurate due to dataclasses getting sorted, this abandons the heuristics altogether, explicitly requesting the source root for the `python_distribution` target. This time, I added a test to make sure direct_url.json has the contents we expect. Note: This is ultimately a cosmetic bug. The direct_url gets printed in the output of `pip list`, but is otherwise not really exposed anywhere. So, it's odd when it's the wrong directory, but does not break any functionality.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
We were using the first entry in
build_time_source_rootsto populate the editable wheel'sdirect_url. However, that does not work as I expected it to becausebuild_time_source_rootsgets sorted before creating theDistBuildRequest, so the first entry is not actually thesource_rootof thepython_distributiontarget.To fix this, we capture the dist's source_root before the source_roots get sorted and use that when building the PEP660 editable wheel.
Functionally, the editable wheels get installed just fine. However, the output of
pip listin an exported venv might show the wrong path if there happens to be a source_root that sorted before the source_root of the distribution. Importing the python code still works, however, as all of the source_roots are included in the.pthfile. Thus, this is a cosmetic / correctness bug.This is the relevant section of the
pip listoutput for the StackStorm project so you can see how odd it looks to have the wrong path showing: