atlassian-api / atlassian-python-api

Atlassian Python REST API wrapper
https://atlassian-python-api.readthedocs.io
Apache License 2.0
1.29k stars 642 forks source link

Use requirements file for install_requires #1298

Closed zbika73 closed 5 months ago

zbika73 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

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 commented 5 months ago

Codecov Report

All modified and coverable lines are covered by tests :white_check_mark:

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

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

Additional details and impacted files ```diff @@ 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 ```

:umbrella: View full report in Codecov by Sentry.
:loudspeaker: Have feedback on the report? Share it here.

FCamborda commented 5 months ago

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.

FCamborda commented 5 months ago

Related to this PR: https://github.com/atlassian-api/atlassian-python-api/issues/1301 @gonchik can you remove the optional kerberos dependency in a pr and create a new release?

gonchik commented 5 months ago

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

Neodreadlord commented 5 months ago

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 commented 5 months ago

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

Sorry I did not do this before, but I was completely unfamiliar with forks. https://github.com/atlassian-api/atlassian-python-api/pull/1303