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

Support attr directive with static evaluation #1753

Merged
merged 12 commits into from May 17, 2020
Merged

Support attr directive with static evaluation #1753

merged 12 commits into from May 17, 2020

Conversation

jwodder
Copy link
Contributor

@jwodder jwodder commented Apr 27, 2019

As I'm sure we all know, the attr: config directive for the version option in setup.cfg doesn't work when the specified module imports one or more third-party packages, thereby forcing package authors to use a different strategy. Proposals have been made (#1316, #1679, others?) to add a function to setuptools that encapsulates the necessary code for strategy number 1, but these have been rejected, most recently with the comment:

On the other hand, we're already discouraging use of imperative code in setup.py. Most importantly, this behavior doesn't have a comparable syntax in setup.cfg. That is, one cannot specify to load the version from a field in a file through setup.cfg (except for an empty file), meaning it becomes yet another feature that's preventing the adoption of declarative config.

This pull request attempts to address this criticism by implementing the relevant functionality as a setup.cfg directive instead of as a user-visible function.

Summary of changes

  • Implement a literal_attr: directive that can be used in version options in setup.cfg. This directive is like attr:, except it extracts the variable assignment with the ast module, thereby supporting variables in modules with third-party imports at the cost of being unable to support variables that are not assigned constant expressions.

  • Update documentation (The "Minimum Version" column for the version option in the table at https://setuptools.readthedocs.io/en/latest/setuptools.html#metadata has not been updated, as I'm not sure what the next version of setuptools is going to be.)

Pull Request Checklist

  • Changes have tests
  • News fragment added in changelog.d. See documentation for details

@jaraco
Copy link
Member

@jaraco jaraco commented May 15, 2020

I'd rather not add yet another way to load the version. There are already several viable techniques:

  • as an attribute (imported)
  • from a file
  • derived from SCM metadata
  • literal

My favorites are the last two.

I'm also not a fan of the two-word moniker ("literal attr").

Is there a way that attr could be written to support this use_case? I think so. Here's what I'd rather see:

  • first try to read the file using AST. If the value can't be evaluated,
  • fallback to importing the module

@jwodder
Copy link
Contributor Author

@jwodder jwodder commented May 15, 2020

I merged the code for the two directives together like you said, but now some tests in setuptools/tests/test_easy_install.py (specifically, test_setup_requires_with_attr_version()) are failing because (as near as I can tell) they're trying to build projects that aren't in the current directory, and so the AST-parsing code can't find the indicated source files. I'd rather not try to restructure the tests to address this without first getting input & advice from you or another major contributor.

(Some tests in setuptools/tests/test_virtualenv.py are also failing, but that's happening on master as well, and so I'm ignoring them.)

@jaraco
Copy link
Member

@jaraco jaraco commented May 15, 2020

Thanks John. This is looking great. I also am a bit surprised by the behavior. Both the tests (through contexts.tempdir()) and the execution (through sandbox.pushd) should be changing the current directory before running. I tried using --pdb to debug, but unfortunately, when using pdb, all of the context managers exit before I have opportunity to inspect. I'll continue to investigate.

@jaraco
Copy link
Member

@jaraco jaraco commented May 15, 2020

If I add the breakpoint explicitly, I can avoid exiting the generators:

diff --git a/setuptools/config.py b/setuptools/config.py
index 0a2f51e2..447d704d 100644
--- a/setuptools/config.py
+++ b/setuptools/config.py
@@ -352,6 +352,7 @@ class ConfigHandler:
         elif os.path.isdir(fpath):
             fpath = os.path.join(fpath, '__init__.py')
         else:
+            breakpoint()
             raise DistutilsOptionError('Could not find module ' + module_name)
         with open(fpath, 'rb') as fp:
             src = fp.read()

And see what's going on:

> /Users/jaraco/code/public/pypa/setuptools/setuptools/config.py(356)_parse_attr()
-> raise DistutilsOptionError('Could not find module ' + module_name)
(Pdb) os.getcwd()
'/private/var/folders/c6/v7hnmq453xb6p2dbz1gqc6rr0000gn/T/tmp2k2pxot_/test_pkg'
(Pdb) os.listdir('.')
['temp', 'setup.py', 'setup.cfg']

Somehow, foobar.py is missing.

@jaraco
Copy link
Member

@jaraco jaraco commented May 15, 2020

I've figured it out. That test is doing something really complicated.

It's creating a package test_pkg which has a setup_requires requirement on foobar==0.1 and in foobar 0.1 there is a foobar module with version = 42 and the test_pkg is defining its version as coming from the attribute foobar.version. That's why it works when you import foobar; foobar.version but not so much if you assume that foobar is in the current directory.

The test has done its job and caught a real use-case that it's trying to protect.

Next will be to figure out how to "find" a module without loading it (hint: Python has "finders" and "loaders" defined in PEP 302, though there's probably something better in later peps, probably PEP 451 and importlib.util.find_spec) so as to honor the import path for finding the relevant source.

@jaraco jaraco changed the title Implement literal_attr: config directive Support attr directive with static evaluation May 16, 2020
@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

This latest version is almost working. The tests are passing, but I'm not sure how. The test that's there should probably be a separate unit test and not just some assertions on an existing test. I've removed the specialized tuple handling, but the test is still passing. That shouldn't be the case if the test is effectively capturing the feature.

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

On second thought, I see that you're following the pattern of the test suite, so the test may be okay. But it definitely needs to fail before the patch is applied.

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

That's really interesting. The tests are passing on macOS but not Windows or LInux. Seems find_spec is returning None.

        spec = importlib.util.find_spec(name)
>       with open(spec.origin) as strm:
E       AttributeError: 'NoneType' object has no attribute 'origin'

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

I'll be the issue is that the contents of fake_package.subpackage are probably cached, so adding a module after importing isn't supported except on macOS.

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

Deleting the packages from sys.modules allowed the Windows tests to start passing. Really feels like this behavior is inscrutable.

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

Well, hell. This latest technique works on Ubuntu Python 3.6, but revives the failure on Windows.

@jaraco
Copy link
Member

@jaraco jaraco commented May 17, 2020

It looks like the last remaining failure is on Python 2. I was going to try to add Python 2 compatibility by pulling in importlib2, but then I found that project is broken and abandoned since 2015. So no Python 2 support for this one.

@jaraco jaraco merged commit c457e68 into pypa:master May 17, 2020
14 of 22 checks passed
@merwok
Copy link
Contributor

@merwok merwok commented May 17, 2020

I think importlib2 was a wholesale backport, these days there are separate projects like importlib_metadata and importlib_resources to backport submodules of stdlib importlib.

@jwodder
Copy link
Contributor Author

@jwodder jwodder commented May 17, 2020

Thanks for the help, @jaraco!

@JulienPalard
Copy link
Contributor

@JulienPalard JulienPalard commented Jun 5, 2020

Thanks a lot @jwodder for implementing this!!

sqlalchemy-bot pushed a commit to sqlalchemy/sqlalchemy that referenced this issue Aug 13, 2020
The ``importlib_metadata`` library is used to scan for setuptools
entrypoints rather than pkg_resources.   as importlib_metadata is a small
library that is included as of Python 3.8, the compatibility library is
installed as a dependency for Python versions older than 3.8.

Unfortunately setuptools "attr:" is broken because it tries to import
the module; seems like this is fixed as part of
pypa/setuptools#1753 however this is too recent
to rely upon for now.

Added a new dialect token "mariadb" that may be used in place of "mysql" in
the :func:`_sa.create_engine` URL.  This will deliver a MariaDB dialect
subclass of the MySQLDialect in use that forces the "is_mariadb" flag to
True.  The dialect will raise an error if a server version string that does
not indicate MariaDB in use is received.   This is useful for
MariaDB-specific testing scenarios as well as to support applications that
are hardcoding to MariaDB-only concepts.  As MariaDB and MySQL featuresets
and usage patterns continue to diverge, this pattern may become more
prominent.

Fixes: #5400
Fixes: #5496
Change-Id: I330815ebe572b6a9818377da56621397335fa702
graingert added a commit to graingert/exceptiongroup that referenced this issue Nov 4, 2020
graingert added a commit to graingert/exceptiongroup that referenced this issue Nov 4, 2020
graingert added a commit to graingert/exceptiongroup that referenced this issue Nov 4, 2020
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.

None yet

4 participants