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

Move setup properties out of setup.py in to setup.cfg #12417

Merged
merged 4 commits into from
Nov 18, 2020

Conversation

ashb
Copy link
Member

@ashb ashb commented Nov 17, 2020

I've moved all the ones that are "static" -- any form of dynamic or interpolated values are left in setup.py

If a value is passed as n kwrg to setup and in setup.cfg, the kwarg wins out.


^ Add meaningful description above

Read the Pull Request Guidelines for more information.
In case of fundamental code change, Airflow Improvement Proposal (AIP) is needed.
In case of a new dependency, check compliance with the ASF 3rd Party License Policy.
In case of backwards incompatible changes please leave a note in UPDATING.md.

@ashb ashb requested a review from potiuk November 17, 2020 21:04
@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

I'm just moving across install_reuirements too

@potiuk
Copy link
Member

potiuk commented Nov 17, 2020

I had a quick look before going to bed - and there is nothing that I would not like in this change @ashb :). As long as all the cases we had (installing locally, installing via Github llnk, installing via PyPI) work - this is all fine.

There are a few (or maybe just one - INSTALL_REQUIREMENTS order - check_install_and_setup_requires in pre_commit_check_order_setup.py ) static check/pre-commit that you will find that need updating. I think that's all - but we should also carefully look at the artifacts generated by the PR to be sure of that (especially whether the docker images built contain all providers and whether the provider packages have the right dependencies etc.

Nothing controversial in this PR really :)

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

Yeah the possible contriversal stuff (version detection, extra requires) needed more brain power to think of that I have tonight. So I'm doing sort of mechanical easy-to-do changes :D

@ashb
Copy link
Member Author

ashb commented Nov 17, 2020

Updated the sort checking in pre-commit. Setup tool provides a read_configuration function we can use.

@@ -779,7 +779,8 @@ jobs:
cache-name: cache-kubernetes-tests-v7-master
with:
path: ".build/.kubernetes_venv*"
key: "venv-${{ env.cache-name }}-${{ github.job }}-${{ hashFiles('setup.py') }}\
key: "venv-${{ env.cache-name }}-${{ github.job }}-${{ hashFiles('setup.py')\
-${{ hashFiles('setup.cfg') }}\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good point!

@@ -27,7 +27,7 @@ Getting Airflow
Airflow is published as ``apache-airflow`` package in PyPI. Installing it however might be sometimes tricky
because Airflow is a bit of both a library and application. Libraries usually keep their dependencies open and
applications usually pin them, but we should do neither and both at the same time. We decided to keep
our dependencies as open as possible (in ``setup.py``) so users can install different version of libraries
our dependencies as open as possible (in ``setup.cfg``) so users can install different version of libraries
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Should not that be (for now at least) both setup.py and setup.cfg?

setup.cfg Outdated

[options.packages.find]
exclude =
airflow.providers
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe find: uses "find_packages" not the namespace ones (there is a separate find_namespace:)? So we probably do not need those "excludes" here.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I just did a lift-and-shift of the find_packages() call from setup.py. You are right though.

Copy link
Member

@potiuk potiuk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Love it! Few small Nits/

@@ -788,6 +789,7 @@ jobs:
with:
path: ".build/bin"
key: "bin-${{ env.cache-name }}-${{ github.job }}-${{ hashFiles('setup.py') }}\
-${{ hashFiles('setup.cfg') }}\
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actually here we can remove both setup.py and setup.cfg. The ./buld/bin content only depends on the version of tools used (helm//kind/kubectl) and it does not depend on setup.py nor setup.cfg in any way. That was probably a copy&paste problem.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Will do. (I didn't check the logic here, just saw setup.py being fingerprinted and added setup.cfg)

@github-actions github-actions bot added the full tests needed We need to run full set of tests for this PR to merge label Nov 18, 2020
@github-actions
Copy link

The PR needs to run all tests because it modifies core of Airflow! Please rebase it to latest master or ask committer to re-run it!

I've moved all the ones that are "static" -- any form of dynamic or
interpolated values are left in setup.py

If a value is passed as n kwrg to setup and in setup.cfg, the kwarg
wins out.
@ashb ashb merged commit f034d4b into apache:master Nov 18, 2020
@ashb ashb deleted the static-info-to-setupcfg branch November 18, 2020 13:23
potiuk added a commit to PolideaInternal/airflow that referenced this pull request Nov 18, 2020
The PR apache#12417 broke CI.yaml accidentally. This PR fixes it.
@potiuk potiuk mentioned this pull request Nov 18, 2020
potiuk added a commit that referenced this pull request Nov 18, 2020
The PR #12417 broke CI.yaml accidentally. This PR fixes it.
potiuk added a commit that referenced this pull request Nov 29, 2020
The PR #12417 broke CI.yaml accidentally. This PR fixes it.

(cherry picked from commit 93b3270)
kaxil pushed a commit that referenced this pull request Dec 3, 2020
The PR #12417 broke CI.yaml accidentally. This PR fixes it.

(cherry picked from commit 93b3270)
ashb pushed a commit that referenced this pull request Dec 3, 2020
The PR #12417 broke CI.yaml accidentally. This PR fixes it.

(cherry picked from commit 93b3270)
cfei18 pushed a commit to cfei18/incubator-airflow that referenced this pull request Mar 5, 2021
The PR apache#12417 broke CI.yaml accidentally. This PR fixes it.

(cherry picked from commit 93b3270)
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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants