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

Add missing keyring dep in setup.py and pyproject.toml #182

Closed
wants to merge 1 commit into from

Conversation

tucksaun
Copy link

@tucksaun tucksaun commented Feb 15, 2024

This PR

Add missing keyring dep in setup.py and pyproject.toml.

  • I've read the DCO.
  • I've read the Coding Guidelines
  • The relevant informations about the changes stands in the commit message, not here in the message of the pull request.
  • Code changes follow the style of the files they change.
  • Code is tested (pip install --no-cache-dir -v git+https://github.com/tucksaun/offlineimap3.git@master works successfully + GHA).

References

Additional information

#102 added support of system keyring and added keyring to requirements.txt but not in setup.py nor pyproject.toml. As a result the installation of offlineimap using pip pip install --no-cache-dir -v git+https://github.com/OfflineIMAP/offlineimap3.git@master does not work anymore.
Adding the missing dependency to those files fixes it.

@etbuira
Copy link

etbuira commented Jul 25, 2024

Looks good, but should IMHO be merged with PR #191

@WhyNotHugo
Copy link
Contributor

This should probably be an optional dependency, not a strict one; that whole dependency tree isn't necessary unless the keyring feature is being used.

@etbuira
Copy link

etbuira commented Jul 25, 2024

@WhyNotHugo sure, but shouldn't pyproject.toml be updated as well? (in #191)

@thekix
Copy link
Member

thekix commented Aug 15, 2024

Hi,

Thanks a lot for your patch. Because I included the #191, probably we can close this pull request.

@tucksaun do you agree?

Best regards,
kix

@WhyNotHugo
Copy link
Contributor

@WhyNotHugo sure, but shouldn't pyproject.toml be updated as well? (in #191)

You're right, it SHOULD be added to pyproject.toml, although as an optinal dependency. See here for an example of the exact syntax.

@etbuira
Copy link

etbuira commented Aug 16, 2024

It SHALL, as well as

@thekix
Copy link
Member

thekix commented Aug 17, 2024

@WhyNotHugo sure, but shouldn't pyproject.toml be updated as well? (in #191)

You're right, it SHOULD be added to pyproject.toml, although as an optinal dependency. See here for an example of the exact syntax.

Dear @WhyNotHugo,

I created this pyproject.toml. Before create the patch, I would like to discuss about it, to include all the information. The proposal file is:

[project]
name = "offlineimap"
version = "3.0.0"
description = "IMAP synchronization tool"
authors = [
    { name = "John Goerzen & contributors", email = "[email protected]" }
]
readme = "README.rst"
license = { text = "GPL-2.0" }
readme = "README.md"
homepage = "http://www.offlineimap.org"
keywords = ["client", "imap", "cli", "email", "mail", "synchronization", "sync", "offline"]
requires-python = ">=3.7"
classifiers = [
    "Development Status :: 5 - Production/Stable",
    "Environment :: Console",
    "License :: OSI Approved :: GNU General Public License v2 (GPLv2)",
    "Operating System :: POSIX",
    "Programming Language :: Python :: 3",
    "Programming Language :: Python :: 3.8",
    "Programming Language :: Python :: 3.9",
    "Programming Language :: Python :: 3.10",
    "Programming Language :: Python :: 3.11",
    "Programming Language :: Python :: 3.12",
    "Programming Language :: Python :: Implementation :: PyPy",
    "Topic :: Office/Business :: Scheduling",
    "Topic :: Utilities",
]

[project.urls]
homepage = "http://www.offlineimap.org"
documentation = "https://www.offlineimap.org/documentation.html"
issues = "https://github.com/OfflineIMAP/offlineimap3/issues"
repository = "https://github.com/OfflineIMAP/offlineimap3/"

[build-system]
requires = [
    "setuptools>=18.5",
    "wheel"
]

[project.dependencies]
distro = "*"
imaplib2 = ">=3.5"
rfc6555 = "*"
urllib3 = "~=1.25.9"

[project.optional-dependencies]
keyring = ["keyring"]
cygwin = ["portalocker[cygwin]"]
kerberos = ["gssapi[kerberos]"]
testinternet = ["certifi~=2020.6.20"]

[project.scripts]
offlineimap = "bin/offlineimap"

About the requirements file, we can include the keyring as optional, as:

# Requirements
gssapi[kerberos]
portalocker[cygwin]
rfc6555
distro;platform_system=="Linux" and python_version>"3.7"
keyring[keyring]
imaplib2>=3.5
urllib3~=1.25.9
testinternet[certifi~=2020.6.20]

About the setup.py, move cygwin to extra_require and create testinternet with certify

install_requires=['distro', 'imaplib2>=3.5', 'rfc6555', 'gssapi[kerberos]', 'urllib3~=1.25.9'],
extras_require={'keyring': ['keyring'], 'cygwin': 'portalocker[cygwin]', 'testinternet':'certifi~=2020.6.20'},

What do you think?

The certifi package is only used by the file contrib/internet-urllib3.py, therefore we can include it as optional dependency.

Regards,
kix

@thekix
Copy link
Member

thekix commented Aug 26, 2024

Hi,

I included a big patch with the setup process. The commit is: 4f5b252

Thanks a lot.
Regards,
kix

PS: This is the commit message:

Full packaging review

This patch includes a lot of changes:

  1. Split the requirements.txt file in multiple files. This change holds
    the required packages for OfflineIMAP in the requirements.txt file.
    The optional packages are included in the files requirements-option.txt
    files. Now the standard OfflineIMAP configuration does not include
    packages like cygwin. See for example issue KeyError: 'cygwin' when installing #192.

  2. The setup.py process includes a lot of files (see issue setup.py imports too much #110). This
    creates a problem in the setup process, because some libraries are not
    found (see Unable to run setup.py from a clean checkout #39, the problem still happends). For this reason we can
    read the variables from the offlineimap/__init__py to include them
    in the setup.py script, without import the offlineimap module. I
    used the method presented at https://www.youtube.com/watch?v=fHNhhHMUW7k.
    In the setup module we don't need the testing code (it creates the import
    problem too), so this code is removed. To read the variables, we use
    some regex search in the offlineimap/__init__py file, save the
    values as variables, and then use them in the setup() call.

  3. setup.py uses requires and extra_requires libraries, aligned with
    the requirements files. We use four different options: kerberos,
    keyring, cygwin, cygwin and testinternet.

  4. pyproject.toml. This file is fully rewritten. The file use now the
    right dependencies, includes the optional dependencies aligned with
    the requirements and the setup.py files. The file include other
    details, like classifiers, URLs,... This script uses now the the
    project.scripts option, with the module and the method to call when
    the setup file is created. Then, this script includes as module
    offlineimap.init, and the startup method is main. Because this
    method is new, this method and the __main__ functions are created
    in the offlineimap/init.py file:

     ```python
     def main():
         oi = OfflineImap()
         oi.run()
    
     if __name__ == "__main__":
         main()
     ```
    

With these changes, the setup process works fine, with and without
optional modules. Finally, the folder offlineimap.egg-info,
created in the setup process is included in the .gitignore file.

It is possible check the creation using:

python -m pip install .

And then use the command offilineimap to use the module. Finally, the
bin/offlineimap command is not used, so we probably can remove it.

Fix: #192, Fix: #110, Fix: #39, Fix: #90

@thekix thekix closed this Aug 26, 2024
@tucksaun
Copy link
Author

tucksaun commented Sep 2, 2024

Hi,

Thanks a lot for your patch. Because I included the #191, probably we can close this pull request.

@tucksaun do you agree?

Best regards, kix

Sorry for the delay in my answer: I was on holiday.

Makes sense indeed, thank you.

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.

4 participants