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

Optionally allow a cwd for get_distribution_version_string #25

Merged
merged 2 commits into from
Jun 7, 2022

Conversation

DMRobertson
Copy link
Contributor

@DMRobertson DMRobertson commented Jun 6, 2022

Needed for matrix-org/synapse#12968.

Since we now use poetry-core to build distributions for Synapse, the locate_file(".") trick no longer seems to work to find Synapse's source directory.

In principle we could do something like

  • have the get-a-version function accept a module object
  • look up the distribution name given the module's __name__ in importlib.metadata.packages_distributions(), and use that to get a version as today
  • get cwd from the module's __file__ attribute and use that for the git invocations

Unfortunately the second step is brittle. When I tried to test this on matrix.org's configuration, packages_distributions()["synapse"] gave me a KeyError for my troubles.

Instead, keep things simple and have the caller give us an optional cwd.

If accepted, I will do a minor release of matrix-python-common and change Synapse to depend on it in matrix-org/synapse#12973 (which is presently a WIP).

to workaround psf/black#2964
@DMRobertson DMRobertson force-pushed the dmr/versionstring-explicit-cwd branch from d4504da to 1b58254 Compare June 6, 2022 18:16
@DMRobertson DMRobertson marked this pull request as ready for review June 6, 2022 18:18
@DMRobertson DMRobertson requested a review from a team as a code owner June 6, 2022 18:18
@DMRobertson DMRobertson marked this pull request as draft June 6, 2022 18:40
DMRobertson pushed a commit to matrix-org/synapse that referenced this pull request Jun 6, 2022
Feels a bit icky, but this is hopefully good enough.
@DMRobertson DMRobertson force-pushed the dmr/versionstring-explicit-cwd branch from aaa6c39 to c27e6d8 Compare June 6, 2022 19:14
@DMRobertson DMRobertson marked this pull request as ready for review June 6, 2022 19:15
Copy link
Contributor

@squahtx squahtx left a comment

Choose a reason for hiding this comment

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

lgtm!

@DMRobertson DMRobertson merged commit 4b0a3e7 into main Jun 7, 2022
@DMRobertson DMRobertson deleted the dmr/versionstring-explicit-cwd branch June 7, 2022 09:42
Comment on lines +50 to +51
cwd: if provided, the directory to run all git commands in. `cwd` may also be
the path to a file, in which case `os.path.dirname(cwd)` is used instead.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I definitely had code which did this. Must have lost it when force pushing. Will fix on main branch and do another release.

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