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

Unpin hatchling #575

Merged
merged 2 commits into from
Feb 5, 2024
Merged

Unpin hatchling #575

merged 2 commits into from
Feb 5, 2024

Conversation

hynek
Copy link
Member

@hynek hynek commented Feb 5, 2024

It's breaking the build. I don't think such a pin on a package we don't control makes any sense. As we're currently seeing, it can break the build just like an incompatible update.

It's breaking the build.
@hynek hynek requested a review from a team as a code owner February 5, 2024 06:15
Copy link
Member

@glyph glyph left a comment

Choose a reason for hiding this comment

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

It's breaking the build. I don't think such a pin on a package we don't control makes any sense. As we're currently seeing, it can break the build just like an incompatible update.

Not sure what you mean by "package we don't control" means in this context, given that we don't control most of the dependencies that need to be pinned? I'm going to assume that the breakage has something to do with the service interface to PyPI, but given that trunk is failing, let's just merge this and discuss heady engineering philosophy later :)

@hynek
Copy link
Member Author

hynek commented Feb 5, 2024

It's breaking the build. I don't think such a pin on a package we don't control makes any sense. As we're currently seeing, it can break the build just like an incompatible update.

Not sure what you mean by "package we don't control" means in this context, given that we don't control most of the dependencies that need to be pinned?

Well, we kinda do control incremental, no?

@hynek hynek merged commit 28bb011 into trunk Feb 5, 2024
15 checks passed
@hynek hynek deleted the unpin-hatchling branch February 5, 2024 08:05
@glyph
Copy link
Member

glyph commented Feb 5, 2024

Well, we kinda do control incremental, no?

Sure, yes. But that should mean we need to pin it less, right? Because surely we won't screw anything up ;-)

@adiroiban
Copy link
Member

My understanding is that hatching is only use to build the package. It is not used at install time or run time.

So I think it's safe not to pin it.
If something breaks, it only breaks for developer, not for users.
And when it breaks, we can pin ... or just wait for upstream to be fixed by itself :)

@glyph
Copy link
Member

glyph commented Feb 5, 2024

If something breaks, it only breaks for developer, not for users.

In principle, everything should be pinned all the time :). Things should not break for developers, because debugging unrelated complex CI and build issues can burst the fragile bubble of contributor motivation.

However, as we can see here, pinning haphazardly or incompletely can be worse than not pinning at all; and in some cases (for example, pinning the codecov client) pinning code that directly depends on external resources can cause more unrelated failures than it prevents. So while principle is a nice place to visit, it seems we cannot live there.

I would like to understand why hatchling in particular failed. My guess is dependency skew, where we forgot to pin something in the transitive dependency chain? In any case, ~= is not really a "pin", since semver is make-believe nonsense; pins should be maintained in a lockfile of some kind, not in pyproject.toml.

@adiroiban
Copy link
Member

This is the failed build

https://github.com/twisted/towncrier/actions/runs/7776065670/job/21202764306#step:3:176

and this is the previous build that was succesfull

https://github.com/twisted/towncrier/actions/runs/7379533493/job/20075901585#step:3:175

Looks like the different is

-cryptography-41.0.7
+cryptography-42.0.2

somehow, poor towncrier build process depends on cryptography

@glyph
Copy link
Member

glyph commented Feb 5, 2024

Looks like the different is

Thanks for tracking that down.

somehow, poor towncrier build process depends on cryptography

I can think of a few scenarios for why this might be (builds really do need to hash stuff or talk to servers securely sometimes), but I wonder which one it actually is, or if it's just a random accident :)

@adiroiban
Copy link
Member

For reference. The hatching bug is here pypa/hatch#1221

@glyph
Copy link
Member

glyph commented Feb 9, 2024

For reference. The hatching bug is here pypa/hatch#1221

I still don't quite understand how this interacts with cryptography?

@adiroiban
Copy link
Member

I think cryptography is a red herring ... it was the only dependency difference reported by the builds.

I have no idea why it failed... I can see both using Python 3.12.1 ... so I don't know

@hynek
Copy link
Member Author

hynek commented Feb 10, 2024

From what I see the problem was the classic “pin only top-level dependency”. Pluggy broke Hatch and we didn’t get the update to fix it?

@adiroiban
Copy link
Member

In the build logs, I don't see pluggy installed...so I don't know... might be the same root cause, but for use it failed in a different way.

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