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

Dont fail on wheels without Requires-Dist lines #21

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

DBS-ST-VIT
Copy link

This is a fix that solves the problem, that was described in #20

@DBS-ST-VIT DBS-ST-VIT changed the title Dont fail on wheels without Requires-Dist lines Draft: Dont fail on wheels without Requires-Dist lines May 28, 2024
@DBS-ST-VIT DBS-ST-VIT marked this pull request as draft May 28, 2024 14:17
@DBS-ST-VIT DBS-ST-VIT closed this May 28, 2024
@DBS-ST-VIT DBS-ST-VIT reopened this May 28, 2024
@DBS-ST-VIT DBS-ST-VIT marked this pull request as ready for review May 28, 2024 14:23
@DBS-ST-VIT DBS-ST-VIT changed the title Draft: Dont fail on wheels without Requires-Dist lines Dont fail on wheels without Requires-Dist lines May 28, 2024
@DBS-ST-VIT DBS-ST-VIT force-pushed the patch-1 branch 2 times, most recently from 6d6f9da to 40edbfa Compare May 29, 2024 06:11
Copy link
Member

@ajkerrigan ajkerrigan left a comment

Choose a reason for hiding this comment

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

Thanks for the report and fix! Appreciate the no-deps test package - I'm inclined to say that it's not worth freezing a wheel with no dependencies, but that also shouldn't break.

A couple suggestions inline, not critical.

Comment on lines 235 to 243
requires_dist = dist_meta.get_all("Requires-Dist")
if requires_dist:
for m in requires_dist:
if not start_pos:
start_pos = dist_meta._headers.index(("Requires-Dist", m))
dist_meta._headers.remove(("Requires-Dist", m))

for idx, h in enumerate(dep_lines):
dist_meta._headers.insert(start_pos + idx, ("Requires-Dist", h))
Copy link
Member

Choose a reason for hiding this comment

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

Good catch here! Though I think if we don't have any dependencies to freeze, we can probably avoid calling replace_deps() in the first place by making a change later in the file:

@@ -296,7 +295,8 @@ class IcedPoet:
             deps = self.get_path_deps(MAIN_GROUP)
             deps.update(dep_packages)
             dep_lines = self.get_frozen_deps(deps, self.exclude_packages)
-            self.replace_deps(dist_meta, dep_lines)
+            if dep_lines:
+                self.replace_deps(dist_meta, dep_lines)
 
             with source_whl.open(record_path) as record_fh:
                 record_text = self.freeze_record(record_fh, dist_meta, md_path)

It's still a good call to play it safe and avoid the chance for a None here, but we should still be able to cover that without indenting all of this logic:

Suggested change
requires_dist = dist_meta.get_all("Requires-Dist")
if requires_dist:
for m in requires_dist:
if not start_pos:
start_pos = dist_meta._headers.index(("Requires-Dist", m))
dist_meta._headers.remove(("Requires-Dist", m))
for idx, h in enumerate(dep_lines):
dist_meta._headers.insert(start_pos + idx, ("Requires-Dist", h))
for m in dist_meta.get_all("Requires-Dist", ()):
if not start_pos:
start_pos = dist_meta._headers.index(("Requires-Dist", m))
dist_meta._headers.remove(("Requires-Dist", m))
for idx, h in enumerate(dep_lines):
dist_meta._headers.insert(start_pos + idx, ("Requires-Dist", h))

Copy link
Author

Choose a reason for hiding this comment

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

Good Morning and thanks for your quick response!
I absolutely agree with your suggestions, which is why i added them already into my PR.

I'm inclined to say that it's not worth freezing a wheel with no dependencies, but that also shouldn't break.

Of course - i absolutely understand that point. But we are facing a edge case on our side - theres a pipeline template which is executing poetry build and poetry freeze-wheel for all of our python projects. Of course, we can put the logic also into the pipeline with some greping and if-conditions, which actually looked very hacky to us - thats the reason, why we decided to contribute the fix into this project. :)

@ajkerrigan
Copy link
Member

Tests are failing here due to code coverage upload failures. I'll get a separate PR together for that, we've had success in other projects moving from a manual upload to a supported GitHub action.

@DBS-ST-VIT
Copy link
Author

Heyho @ajkerrigan ,
are there any news regarding to this PR/the mentioned CI problems?

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.

2 participants