Skip to content

Conversation

ldionne
Copy link
Member

@ldionne ldionne commented Oct 6, 2025

That way, we have a single place where all of our dependency versions are pinned, and setup.py only mentions which packages the tool depends on, without requiring specific versions.

Also drop packages that are not depend upon directly from setup.py: some of those like pytz are still pinned in requirements.txt for stability, but they don't need to be in setup.py because we don't depend on them directly. The six package was dropped entirely since it wasn't used anymore.

Finally, this patch pins the versions used when running tox by installing via requirements.txt, otherwise we end up using unpinned versions from setup.py when running the tests, which creates obvious issues.

That way, we have a single place where all of our dependency versions
are pinned, and setup.py only mentions which packages the tool depends
on, without requiring specific versions.

Also drop packages that are not depend upon directly from setup.py:
some of those like `pytz` are still pinned in requirements.txt for
stability, but they don't need to be in setup.py because we don't
depend on them directly. The `six` package was dropped entirely since
it wasn't used anymore.

Finally, this patch pins the versions used when running `tox` by
installing via requirements.txt, otherwise we end up using unpinned
versions from setup.py when running the tests, which creates obvious
issues.
Copy link

@boomanaiden154 boomanaiden154 left a comment

Choose a reason for hiding this comment

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

LGTM (although I'm not that familiar with setup.py or tox). We should probably add a requirements lockfile at some point too to ensure the transitive dependency closure is maintained.

@ldionne
Copy link
Member Author

ldionne commented Oct 6, 2025

LGTM (although I'm not that familiar with setup.py or tox). We should probably add a requirements lockfile at some point too to ensure the transitive dependency closure is maintained.

Actually, I think that would probably replace our current requirements.txt since AFAICT locking down dependencies is its only purpose.

@ldionne ldionne merged commit 47bc80b into llvm:main Oct 6, 2025
1 of 3 checks passed
@ldionne ldionne deleted the review/sort-dependencies branch October 6, 2025 17:54
Copy link
Contributor

@fhahn fhahn left a comment

Choose a reason for hiding this comment

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

As is, this breaks installing via setup.py, because now we install the latest version of each package, which doesn't work (it will install a version of MarkupSafe that doesn't work with the version it install of Jinja2)

fhahn added a commit that referenced this pull request Oct 7, 2025
Without pinned requirements, we end up installing
incompatible versions of MarkupSafe and Jinja2. See
#63 (review)

For now, restore the pinned version before 47bc80b to unblock
workflows using setup.py.
@fhahn
Copy link
Contributor

fhahn commented Oct 7, 2025

For now I restored the pinned versions in setup.py (8fc27f4), to unblock workflows using setup.py. Not sure what the best solution is, but if the default install/setup via setup.py does not work (and should not be used) we should remove that option

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