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

Use requirements file for install_requires #1298

Conversation

zbika73
Copy link
Contributor

@zbika73 zbika73 commented Jan 16, 2024

Let's define install_requires inside setup.py out of requirements.txt file.
We keep then only one source of dependencies and we let build fail early (on wheel build) if requirements.txt contains wrong dependencies.

@FCamborda
Copy link
Contributor

I'd advise against this solution, since setup.py is not desired for this usage. Instead, the project should use setup.cfg or pyproject.toml and pin dependencies in a requirements.txt only for debugging purposes.

For more info see https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/

@zbika73
Copy link
Contributor Author

zbika73 commented Jan 16, 2024

I'd advise against this solution, since setup.py is not desired for this usage

Well, not entirely true it's not desired.
See:
*) https://packaging.python.org/en/latest/discussions/setup-py-deprecated/
*) https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/#install-requires

For more info see https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/

This PR is not about replacing anything with requirements file, but it feeds install_requires with content of requirements file.
This field (install_requires) is in use today and is not up-to-date (e.g. beautifulsoup4 is missing).
My proposal is just to avoid mistakes: developers usually update TXT and forget changes inside setup.py

If you install atlassian-python-api (let' say version 3.41.0) on clean venv, you will notice library beautifulsoup4 is not installed.
With my update --> all required libraries are installed as expected (I checked!)

Are there any close plans to get rid of all details inside setup.py and use setup.cfg?
If not, then maybe it's worth to apply my proposal and be on safe side until other solution is delivered...

@codecov-commenter
Copy link

Codecov Report

All modified and coverable lines are covered by tests ✅

Comparison is base (4f8c6f1) 34.36% compared to head (dffc369) 34.36%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master    #1298   +/-   ##
=======================================
  Coverage   34.36%   34.36%           
=======================================
  Files          45       45           
  Lines        8241     8241           
  Branches     1146     1146           
=======================================
  Hits         2832     2832           
  Misses       5295     5295           
  Partials      114      114           

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@FCamborda
Copy link
Contributor

I'd advise against this solution, since setup.py is not desired for this usage

Well, not entirely true it's not desired. See: *) https://packaging.python.org/en/latest/discussions/setup-py-deprecated/ *) https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/#install-requires

For more info see https://packaging.python.org/en/latest/discussions/install-requires-vs-requirements/

This PR is not about replacing anything with requirements file, but it feeds install_requires with content of requirements file. This field (install_requires) is in use today and is not up-to-date (e.g. beautifulsoup4 is missing). My proposal is just to avoid mistakes: developers usually update TXT and forget changes inside setup.py

If you install atlassian-python-api (let' say version 3.41.0) on clean venv, you will notice library beautifulsoup4 is not installed. With my update --> all required libraries are installed as expected (I checked!)

Are there any close plans to get rid of all details inside setup.py and use setup.cfg? If not, then maybe it's worth to apply my proposal and be on safe side until other solution is delivered...

That's up to the maintainer of the project. I was just making people aware about that (unfortunately common) bad practice.

If agreed, I would raise a PR myself porting changes to pyproject.toml instead of this solution, given the triviality of this.

@gonchik gonchik merged commit 3bc66d0 into atlassian-api:master Jan 17, 2024
10 checks passed
@FCamborda
Copy link
Contributor

Related to this PR: #1301
@gonchik can you remove the optional kerberos dependency in a pr and create a new release?

@gonchik
Copy link
Member

gonchik commented Jan 17, 2024

@FCamborda could you send the PR, I will merge it

@Neodreadlord
Copy link

Guys, this change has broken my build. When installing Atlassian now on an Ubuntu-22 container, it fails to complete because it cannot find krb5-config.
due to this being a remote container, I do not have root on. I cannot install libkrb5-dev. So this change actually makes the Atlassian module unusable for me.

Is this due to the addition of the 'optional' kerberos dependency or is there another change that happened that's caused this?

  `  Collecting gssapi>=1.6.0 (from pyspnego[kerberos]->requests-kerberos==0.14.0->atlassian-python-api)
          Downloading gssapi-1.8.3.tar.gz (94 kB)
             ━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━ 94.2/94.2 kB 14.1 MB/s 
        eta 0:00:00
         Installing build dependencies: started
         Installing build dependencies: finished with status 'done'
        Getting requirements to build wheel: started
        Getting requirements to build wheel: finished with status 'error'
        error: subprocess-exited-with-error

× Getting requirements to build wheel did not run successfully.
│ exit code: 1
╰─> [21 lines of output]
    /bin/sh: 1: krb5-config: not found
    Traceback (most recent call last):
      File "/usr/local/lib/python3.10/dist-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 353, in <module>
        main()
      File "/usr/local/lib/python3.10/dist-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 335, in main
        json_out['return_val'] = hook(**hook_input['kwargs'])
      File "/usr/local/lib/python3.10/dist-packages/pip/_vendor/pyproject_hooks/_in_process/_in_process.py", line 118, in get_requires_for_build_wheel
        return hook(config_settings)
      File "/tmp/pip-build-env-e0vzv6d8/overlay/local/lib/python3.10/dist-packages/setuptools/build_meta.py", line 325, in get_requires_for_build_wheel
        return self._get_build_requires(config_settings, requirements=['wheel'])
      File "/tmp/pip-build-env-e0vzv6d8/overlay/local/lib/python3.10/dist-packages/setuptools/build_meta.py", line 295, in _get_build_requires
       self.run_setup()
      File "/tmp/pip-build-env-e0vzv6d8/overlay/local/lib/python3.10/dist-packages/setuptools/build_meta.py", line 311, in run_setup
        exec(code, locals())
      File "<string>", line 109, in <module>
      File "<string>", line 22, in get_output
     File "/usr/lib/python3.10/subprocess.py", line 421, in check_output
        return run(*popenargs, stdout=PIPE, timeout=timeout, check=True,
     File "/usr/lib/python3.10/subprocess.py", line 526, in run
               raise CalledProcessError(retcode, process.args,
                   subprocess.CalledProcessError: Command 'krb5-config --libs gssapi' returned non-zero exit status 127.
      [end of output]`

@FCamborda
Copy link
Contributor

@FCamborda could you send the PR, I will merge it

Sorry I did not do this before, but I was completely unfamiliar with forks.
#1303

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

5 participants