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

Exclude yarn.lock from the whl file #16580

Closed
wants to merge 1 commit into from

Conversation

potiuk
Copy link
Member

@potiuk potiuk commented Jun 22, 2021

Currently the airflow wheel is built with the yarn.lock which
is not actually used by the airflow itself. Having this file in the
docker image causes the clair and trivy scanners to fail.

The yarn.lock however is needed in sdist package that's why it should
not be excluded via MANIFEST.in


^ 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.

@potiuk potiuk requested review from kaxil and ashb June 22, 2021 07:51
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

cc: @uranusjr

@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

Tested it:

./breeze prepare-airflow-packages --package-format both

then

[jarek@penguini:~/code/airflow] main(+3/-0)+ 34s ± tar -tvzf dist/apache-airflow-2.2.0.dev0.tar.gz | grep lock
-rw-r--r-- root/root    314970 2021-06-17 09:31 apache-airflow-2.2.0.dev0/airflow/www/yarn.lock
-rw-r--r-- root/root     11357 2021-06-14 22:56 apache-airflow-2.2.0.dev0/licenses/LICENSE-jqclock.txt
[jarek@penguini:~/code/airflow] main(+3/-0)+ ± unzip -t dist/apache_airflow-2.2.0.dev0-py3-none-any.whl | grep lock
    testing: apache_airflow-2.2.0.dev0.dist-info/LICENSE-jqclock.txt   OK

@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

Note - ./breeze prepare-airflow-packages has the nice benefit over running setup.py that it also performs the necessary cleanup. I find it highly annoying that setup.py bdist_wheel and others have to be always preceeded by manual removal of build and egg-info files, otherwise you can have different files included in the package.

The breeze command makes sure to have all the build deps installed and makes sure to cleanup of everything so that it's always reproducible. Highly recommend.

@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

cc: @uranusjr

@ashb
Copy link
Member

ashb commented Jun 22, 2021

Don't close a PR and re-open a new one please -- just leave a comment.

@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

Ah ok. I thought its already been done before with that PR :)

@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

BTW. This one will need a fix in prod image building as well - I will provide the right fix in a moment

Currently the airflow wheel is built with the yarn.lock which
is not actually used by the airflow itself. Having this file in the
docker image causes the clair and trivy scanners to fail.

The yarn.lock however is needed in sdist package that's why it should
not be excluded via MANIFEST.in

Alse asset compilation only happens when Airflow is installed
from sources. When Airflow is installed from packages, the
assets are already compiled-in during package building.
@potiuk potiuk force-pushed the exclude-yarn-lock-from-wheel branch from 705948d to b1c9bda Compare June 22, 2021 08:28
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

Sorry again for closing the original PR, I just pushed my stuff after testing without too much thinking of it, and then closed the other one. Anyway I also added fix that should fix the PROD image building. Turned out the "asset compilation" step was not really needed (and not possible when we missed yarn.lock) when Airflow is installed from packages. Now wo only compile the assets when airflow in prod image is built from sources (which is basically only when we use breeze to build it to test the latest version).

@potiuk potiuk closed this Jun 22, 2021
@potiuk
Copy link
Member Author

potiuk commented Jun 22, 2021

I mixed sdist with source package. My bad

@potiuk potiuk deleted the exclude-yarn-lock-from-wheel branch July 29, 2022 20:10
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

2 participants