FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Only import setuptools_scm when we are in a matplotlib git repo by timhoffm · Pull Request #23144 · matplotlib/matplotlib · GitHub

Repository navigation

Only import setuptools_scm when we are in a matplotlib git repo - #23144

Merged
jklymak merged 1 commit into
matplotlib:mainfrom
timhoffm:fix-setuptools-scm-import
Jun 11, 2022
Merged

jklymak merged 1 commit into
matplotlib:mainfrom
timhoffm:fix-setuptools-scm-import

Conversation

timhoffm commented May 26, 2022 •
edited
Loading

Copy link
Copy Markdown
Member

Closes #23114, where somebody has installed matplotlib into another
git repo.

Just checking that two levels up is a .git is not enough. We have to make sure this is the matplotlib git repo. I've now added a check that the __init__.py is in the folder lib/matplotlib. Is there maybe even a better check that this is the matplotlib git repo?

timhoffm added this to the v3.5.3 milestone May 26, 2022
timhoffm added the Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions. label May 26, 2022

anntzer commented May 26, 2022

Copy link
Copy Markdown
Contributor

A better(?) approach may be the one at https://stackoverflow.com/a/66869528/378080 (in combination with still checking if '.git' exists): add a .editable-install-marker empty file in lib/matplotlib/, which will not be present in normal installs (as it isn't listed in package_data), but will be present in editable installs.

Or, now that I think of it, a .this-is-a-matplotlib-repo empty file in the source root dir, and then just check for (Path(matplotlib.__file__).parent.parent.parent / ".this-is-a-matplotlib-repo").exists()...

timhoffm force-pushed the fix-setuptools-scm-import branch from 3b0ea0f to 2c02929 Compare May 26, 2022 23:13
Closes matplotlib#23114, where somebody has installed matplotlib into another
git repo.

Copy link
Copy Markdown
Member Author

Thanks, a dedicated file .matplotlib-repo is the most clear signal.

timhoffm force-pushed the fix-setuptools-scm-import branch from 2c02929 to db0b2c9 Compare May 26, 2022 23:14

jklymak commented May 27, 2022

Copy link
Copy Markdown
Member

This is prob fine but is there no other way to get our dev version info than setuptools?

Copy link
Copy Markdown
Member Author

This is prob fine but is there no other way to get our dev version info than setuptools?

Um, yes you only have to call git describe with the right parameters and parse the result. The problem is that the result can vary on the state of your repo and you have to translate to a standardized format of the convention you want to use. That's quite fiddly and why setuptools_scm exists. I don't think we want to get into the business of managing that logic ourselves. We use setuptools_scm only as a dev-dependency, and for devs, it's ok that they install this as well.

QuLogic commented May 27, 2022

Copy link
Copy Markdown
Member

Should you add this file as export-ignore in .gitattributes? That will leave it out of any /archive/* tarballs, which we may want?

Copy link
Copy Markdown
Member Author

Maybe. I have not used archives. From what I've understood this is only relevant for git archive. I assume source distributions do not use this? So the only case would be people doing git archive themeselves. Here I have no clear view what they are trying to achieve. If they are archiving a source checkout only to bring it back later, they'd need the file to be included. If they archive to share and distribute, the file should not be included.

QuLogic commented May 27, 2022

Copy link
Copy Markdown
Member

For example, Fedora builds from https://github.com/matplotlib/matplotlib/archive/v%{Version}/matplotlib-%{Version}.tar.gz, a tarball which GitHub creates using git archive. I think we do install setuptools-scm in the end though. And then tests are run on an installed copy, so as this is relative to the install, maybe it's fine to keep?

timhoffm commented May 28, 2022 •
edited
Loading

Copy link
Copy Markdown
Member Author

TL;DR The PR is good as is. This is only the explanation why.

Thinking about it, we should not exclude .matplotlib-repo from archive. We only use setuptools_scm if both .matplotlib-repo and .git are present. So the existence/missing of .git already handles that under normal circumstances - and in principle I could see both usages for an archive. .matplotllib-repo is only needed as a second level guard. It's only for the case that a not repo version is installed to a location that happens to be a git repo.

anntzer commented Jun 1, 2022

Copy link
Copy Markdown
Contributor

I am happy with the PR as it is (and would not bother with removing .matplotlib-repo from archives, as that's one less config item and leaving it in seems harmless), but would rather let @QuLogic do the second approval as he's the most likely to be impacted.

jklymak requested a review from QuLogic June 2, 2022 10:56
jklymak merged commit 3407cbc into matplotlib:main Jun 11, 2022
meeseeksmachine pushed a commit to meeseeksmachine/matplotlib that referenced this pull request Jun 11, 2022
oscargus added a commit that referenced this pull request Jun 11, 2022
…144-on-v3.5.x

Backport PR #23144 on branch v3.5.x (Only import setuptools_scm when we are in a matplotlib git repo)
timhoffm deleted the fix-setuptools-scm-import branch June 11, 2022 12:54
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters. Learn more about bidirectional Unicode characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Release critical For bugs that make the library unusable (segfaults, incorrect plots, etc) and major regressions.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Bug]: matplotlib __init__.py checks for .git folder 2 levels up, then errors due to setup tools_scm

5 participants


Back | FazBrowse Home | New Git URL