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

Fix build deps compilation for setuptools < 70.1.0 #2106

Draft
wants to merge 3 commits into
base: main
Choose a base branch
from

Conversation

chrysle
Copy link
Contributor

@chrysle chrysle commented Jun 25, 2024

See #1681 (comment) for context.

Contributor checklist
  • Included tests for the changes.
  • PR title is short, clear, and ready to be included in the user-facing changelog.
Maintainer checklist
  • Verified one of these labels is present: backwards incompatible, feature, enhancement, deprecation, bug, dependency, docs or skip-changelog as they determine changelog listing.
  • Assign the PR to an existing or new milestone for the target version (following Semantic Versioning).

@chrysle chrysle added tests Testing and related things pip Related to pip skip-changelog Avoid listing in changelog labels Jun 25, 2024
Copy link
Member

@webknjaz webknjaz left a comment

Choose a reason for hiding this comment

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

This is not a correct fix. It just shoves the problem under the carpet. We need to address the underlying problem, which is revealed not due to pip but because setuptools changed. Yet, the bug is in pip-tools: #2104 (comment). The correct change in the test is to include something like -P 'setuptools < 70.1.0'. But that should be accompanied with a fix for the bug.

@webknjaz
Copy link
Member

webknjaz commented Jul 7, 2024

Alternatively, it's even better to emulate such a situation with artificial package stubs so that it doesn't need to hit the network.

@chrysle chrysle marked this pull request as draft July 17, 2024 14:43
@chrysle chrysle changed the title Fix CI for pip 24.0 Fix build deps compilation for setuptools < 70.1.0 Jul 22, 2024
@chrysle chrysle force-pushed the fix-ci branch 3 times, most recently from e89a2f4 to 66b6382 Compare July 22, 2024 08:12
Co-authored-by: Sviatoslav Sydorenko (Святослав Сидоренко) <578543+webknjaz@users.noreply.github.com>
@chrysle chrysle added bug Something is not working setuptools Related to compiling requirements with `setuptools` build backend and removed tests Testing and related things pip Related to pip skip-changelog Avoid listing in changelog labels Jul 22, 2024
@chrysle chrysle marked this pull request as ready for review July 22, 2024 08:28
@chrysle
Copy link
Contributor Author

chrysle commented Jul 22, 2024

@webknjaz I reworked the PR; please have a look. Of course there are still the failing Windows tests, can we just drop that horrible Microsoft product ;-)?

@chrysle chrysle requested a review from webknjaz July 23, 2024 19:26
@chrysle chrysle added this to the 7.4.2 milestone Jul 28, 2024
@WhyNotHugo
Copy link
Member

Slightly changing the order of assertions should help see more details into the errors:

diff --git a/tests/test_cli_compile.py b/tests/test_cli_compile.py
index c5031fc..8db0038 100644
--- a/tests/test_cli_compile.py
+++ b/tests/test_cli_compile.py
@@ -787,13 +787,13 @@ def test_direct_reference_with_extras(runner):
             "pip-tools[testing,coverage] @ git+https://github.com/jazzband/pip-tools@6.2.0"
         )
     out = runner.invoke(cli, ["-n", "--rebuild", "--no-build-isolation"])
-    assert out.exit_code == 0
     assert (
         "pip-tools[coverage,testing] @ git+https://github.com/jazzband/pip-tools@6.2.0"
         in out.stderr
     )
     assert "pytest==" in out.stderr
     assert "pytest-cov==" in out.stderr
+    assert out.exit_code == 0
 
 
 def test_input_file_without_extension(pip_conf, runner):

Generally, I prefer to compare out.stderr before checking the status code in tests; if stderr isn't the expected output, pytest will print it when failing the assertion.

@webknjaz
Copy link
Member

Generally, I prefer to compare out.stderr before checking the status code in tests; if stderr isn't the expected output, pytest will print it when failing the assertion.

That won't be necessary if you'll follow the principle of a single assertion per test. Then pytest will print out everything. But personally, I have --showlocals in all configs so that the context is always printed..

@webknjaz
Copy link
Member

webknjaz commented Sep 5, 2024

@chrysle the windows failure is because of the deprecation warning in pip, right?

@chrysle
Copy link
Contributor Author

chrysle commented Sep 5, 2024

Could you elaborate on that? It seems that the resolver only found some pre-versions of a package, only I don't know much because the output is shortened.

@webknjaz
Copy link
Member

Yeah, it's weird. Somebody needs to use https://github.com/marketplace/actions/debugging-with-tmate to see what's happening on Windows. The vars in CI seem to suggest that stderr is incomplete or something. Can it be some output buffering specific to windows? Do we need PYTHONUNBUFFERED=1 in tests?

Comment on lines +3459 to +3465
try:
assert out.exit_code == 0
assert expected == out.stdout
except Exception: # pragma: no cover
print(out.stdout)
print(out.stderr)
raise
Copy link
Member

Choose a reason for hiding this comment

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

Let's not include this hack with more complex logic. This should be better:

Suggested change
try:
assert out.exit_code == 0
assert expected == out.stdout
except Exception: # pragma: no cover
print(out.stdout)
print(out.stderr)
raise
assert 0 == out.exit_code and expected == out.stdout, f"\n{out.stdout=}\n\n{out.stderr=}\n\n"

"--constraint",
os.fspath(tmp_path / "constraints.txt"),
"--upgrade-package",
"setuptools < 70.1.0", # setuptools>=70.1.0 doesn't require wheel any more
Copy link
Member

Choose a reason for hiding this comment

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

FWIW, it'd be better to make it not need internet access. But that's not a blocker, just something to think about for the future.

Comment on lines 687 to +688
assert dependency in out.stderr
assert out.exit_code == 0
Copy link
Member

Choose a reason for hiding this comment

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

I'd prefer not to include unrelated changes right in this PR. But if you keep this, it's probably better to merge the instructions into a single assert statement. This way, it'd be following the AAA principle to the letter: one test == one assertion.

Suggested change
assert dependency in out.stderr
assert out.exit_code == 0
assert 0 == out.exit_code and dependency in out.stderr

@webknjaz
Copy link
Member

webknjaz commented Dec 2, 2024

@chrysle could you try rebasing?

@webknjaz webknjaz self-requested a review December 2, 2024 07:42
@webknjaz webknjaz mentioned this pull request Dec 2, 2024
4 tasks
@webknjaz webknjaz closed this Dec 4, 2024
@webknjaz webknjaz reopened this Dec 4, 2024
@webknjaz
Copy link
Member

webknjaz commented Dec 4, 2024

Closed+reopened to re-trigger the CI as the logs got stale and garbage-collected.

@webknjaz
Copy link
Member

webknjaz commented Dec 4, 2024

Of course there are still the failing Windows tests

Interestingly, the “previous” tests pass, even on Windows. But the “latest” crash all over the place because of the relative vs. absolute path differences. “main” fails as well.
“previous” is set to pip==22.2.* currently.

Honestly, I'm leaning towards making the tests more stable by pinning pip in regular push/PR runs and only having it hit newer versions in nightlies...

@webknjaz webknjaz marked this pull request as draft December 4, 2024 03:16
@webknjaz
Copy link
Member

webknjaz commented Dec 4, 2024

I've converted this to draft to prevent accidental merging ahead of time. I'll undraft it once I'm done experimenting.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something is not working setuptools Related to compiling requirements with `setuptools` build backend
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants