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 newer setuptools v67.2.0 #29465

Merged
merged 4 commits into from
Feb 11, 2023
Merged

Use newer setuptools v67.2.0 #29465

merged 4 commits into from
Feb 11, 2023

Conversation

arjunanan6
Copy link
Contributor

@arjunanan6 arjunanan6 commented Feb 10, 2023

Issue #29428 describes a potential CVE in setuptools v63.4.3 that is currently used, and says that any version before 65.5.1 could have the vulnerability.

Switching to the newest release of setuptools, v67.2.0 for this. A local test with breeze didn't indicate any issues after this change.

Resolves #29428

@boring-cyborg
Copy link

boring-cyborg bot commented Feb 10, 2023

Congratulations on your first Pull Request and welcome to the Apache Airflow community! If you have any issues or are unsure about any anything please check our Contribution Guide (https://github.com/apache/airflow/blob/main/CONTRIBUTING.rst)
Here are some useful points:

  • Pay attention to the quality of your code (ruff, mypy and type annotations). Our pre-commits will help you with that.
  • In case of a new feature add useful documentation (in docstrings or in docs/ directory). Adding a new operator? Check this short guide Consider adding an example DAG that shows how users should use it.
  • Consider using Breeze environment for testing locally, it's a heavy docker but it ships with a working Airflow and a lot of integrations.
  • Be patient and persistent. It might take some time to get a review or get the final approval from Committers.
  • Please follow ASF Code of Conduct for all communication including (but not limited to) comments on Pull Requests, Mailing list and Slack.
  • Be sure to read the Airflow Coding style.
    Apache Airflow is a community-driven project and together we are making it better 🚀.
    In case of doubts contact the developers at:
    Mailing List: [email protected]
    Slack: https://s.apache.org/airflow-slack

@arjunanan6
Copy link
Contributor Author

@potiuk @eladkal change is ready for review whenever you have time too look at it. Thank you!

@Taragolis Taragolis added the full tests needed We need to run full set of tests for this PR to merge label Feb 10, 2023
@potiuk
Copy link
Member

potiuk commented Feb 11, 2023

Running tests. Let's see.

@arjunanan6
Copy link
Contributor Author

@potiuk Seems like all tests pass except one for test_internal_api_command.

@potiuk potiuk merged commit 41dff98 into apache:main Feb 11, 2023
@boring-cyborg
Copy link

boring-cyborg bot commented Feb 11, 2023

Awesome work, congrats on your first merged pull request!

@pierrejeambrun pierrejeambrun added this to the Airflow 2.5.2 milestone Feb 27, 2023
@pierrejeambrun pierrejeambrun added the type:misc/internal Changelog: Misc changes that should appear in change log label Feb 27, 2023
pierrejeambrun pushed a commit that referenced this pull request Mar 7, 2023
Co-authored-by: eladkal <[email protected]>
(cherry picked from commit 41dff98)
pierrejeambrun pushed a commit that referenced this pull request Mar 8, 2023
Co-authored-by: eladkal <[email protected]>
(cherry picked from commit 41dff98)
@pankajkoti
Copy link
Member

pankajkoti commented Jun 23, 2023

I am facing issues with this for local virtualenv setup for editable installs. Not sure what we can do here, but for now pinning it locally to 63.4.3 for getting my local virtualenv up.

There's also a comment we have leftover just above the change in this PR which mentions about the issue regarding editable installs.

Since, the PR description mentions fix for a potential vulnerability, I guess we have to live it with like this until setuptools fixes the issue?

@potiuk
Copy link
Member

potiuk commented Jun 23, 2023

We have an issue about it #30764 already and yeah. I think it needs a bit closer look. I think it might be quite tricky to solve - our setup.py is pretty, well complex, so for now I kept on ignoring it and I think we will need to solve it possibly by generally modernizing our package build tool configuration - part of it has been done in draft #31378 by @uranusjr .

But what makes it difficult is that we want to keep the possibilities of:

  1. Being able to run pip install -e . and have all "airfow + providers" in one editable environment

  2. Similarly - have the environment in Breeze where both airflow and providers are immediately "editable" - i.e. installed from sources so that you do not have to reinstall provider package every time you edit the source code of it.

  3. But also have an option to only install airflow, without the providers and install the providers from pypi (for CI and testing and constraint generation).

So far this has been achieved by INSTALL_PROVIDERS_FROM_SOURCES env variable that determines the approach taken (and in breeze for example INSTALL_PROVIDERS_FROM_SOURCES is set to true. There is also quite complex setup.py code that has been handling that, but in recent releases of pip and setuptools some of the hacks we were using to achieve that got subtly broken and we need to figure out another approach.

It might also be that we will only be able to do it "well" when we switch to a "proper" structure for provider's code, following #28291 (scripts) and #28292 (POC migration) - but then solving the above a little contradicting requirements might be tricky.

I've been following what's going on in Python PyPa and maybe we should switch to flit or hatch - both are part of PyPa and both have great maintainers who are working on them. I believe that in hatch (listening to recent podcast of hatch creator: https://talkpython.fm/episodes/show/408/hatch-a-modern-python-workflow there is an idea or maybe even an early implementation already for something similar to our case where you have multiple projects in monorepo that should be edit-installable together.

And my preference would be rather than invent something on our own, to contribute some work to standard tooling to make it capable of doing what we want to do. I think this is a great opportunity if our goals and ideas are aligned, because we could provide a nice testing ground for such case for hatch (for example).

@uranusjr - I think you are the best person here to have a say and recommendations what we should do and likely you even have some good relation with hatch / flit maintainers and we could jointly do something together and maybe even help to set the standards for the Python packaging world ?

We could setup a small team around it and work on both - contributing things to hatch (if the direction would be aligned) and applying it to Airflow at the same time.

Maybe we should brainstorm a little here or in #30764 on what we should do and how we should approach it?

@pankajkoti
Copy link
Member

I have created a quick PR #32090 to update the comment a bit.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
full tests needed We need to run full set of tests for this PR to merge type:misc/internal Changelog: Misc changes that should appear in change log
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Require newer version of pypi/setuptools to remove security scan issue (CVE-2022-40897)
6 participants