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

[AIRFLOW-3697] Vendorize nvd3 and slugify #4513

Open
wants to merge 2 commits into
base: master
from

Conversation

Projects
None yet
5 participants
@bolkedebruin
Copy link
Contributor

bolkedebruin commented Jan 13, 2019

nvd3 has a dependency on python-slugify which pulls in a
GPL dependency by default.

@kaxil

cc: @Fokko @ashb

@bolkedebruin bolkedebruin force-pushed the bolkedebruin:AIRFLOW-3697 branch 2 times, most recently from d4e7c8f to bce1ba4 Jan 13, 2019

@kaxil

This comment has been minimized.

Copy link
Contributor

kaxil commented Jan 14, 2019

I would suggest if this needs more work and probably more time we can include it in the next release. We can have a patch release 1.10.3 after fixing this issue. Let me know your thoughts on this if you agree I will cut RCs for 1.10.2.

CI fails with similar errors as below:

======================================================================
1) ERROR: test_backfill (tests.CliTests)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/core.py line 1073 in setUp
      self.app, self.appbuilder = application.create_app(session=Session, testing=True)
    airflow/www_rbac/app.py line 163 in create_app
      init_views(appbuilder)
    airflow/www_rbac/app.py line 91 in init_views
      from airflow.www_rbac import views
    airflow/www_rbac/views.py line 65 in <module>
      from airflow._vendor import nvd3
    airflow/_vendor/nvd3/__init__.py line 19 in <module>
      from .lineChart import lineChart
    airflow/_vendor/nvd3/lineChart.py line 12 in <module>
      from .NVD3Chart import NVD3Chart, TemplateMixin
    airflow/_vendor/nvd3/NVD3Chart.py line 26 in <module>
      pl = PackageLoader('nvd3', 'templates')
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/jinja2/loaders.py line 224 in __init__
      provider = get_provider(package_name)
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/pkg_resources/__init__.py line 359 in get_provider
      __import__(moduleOrReq)
   ImportError: No module named nvd3

======================================================================
2) ERROR: test_cli_connections_add_delete (tests.CliTests)
----------------------------------------------------------------------
   Traceback (most recent call last):
    tests/core.py line 1073 in setUp
      self.app, self.appbuilder = application.create_app(session=Session, testing=True)
    airflow/www_rbac/app.py line 163 in create_app
      init_views(appbuilder)
    airflow/www_rbac/app.py line 91 in init_views
      from airflow.www_rbac import views
    airflow/www_rbac/views.py line 65 in <module>
      from airflow._vendor import nvd3
    airflow/_vendor/nvd3/__init__.py line 19 in <module>
      from .lineChart import lineChart
    airflow/_vendor/nvd3/lineChart.py line 12 in <module>
      from .NVD3Chart import NVD3Chart, TemplateMixin
    airflow/_vendor/nvd3/NVD3Chart.py line 26 in <module>
      pl = PackageLoader('nvd3', 'templates')
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/jinja2/loaders.py line 224 in __init__
      provider = get_provider(package_name)
    .tox/py27-backend_mysql-env_docker/lib/python2.7/site-packages/pkg_resources/__init__.py line 359 in get_provider
      __import__(moduleOrReq)
   ImportError: No module named nvd3
@ashb

This comment has been minimized.

Copy link
Member

ashb commented Jan 14, 2019

I would like to see this as two non-squashed commits:

  • The first commit vendorising the modules and using them,
  • The second commit that makes any local changes.

This will make any future maintenance or upgrading of these vendored modules easier. (This will mean that we can't merge via the GitHub button, but that's okay.)

I'm happy to do this change if you don't have time (I will on Wednesday)

@BasPH

This comment has been minimized.

Copy link
Contributor

BasPH commented Jan 15, 2019

Out of interest: how does vendorizing a 3rd party lib result in removal of the GPL dependency?

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 15, 2019

@BasPH cause one of the vendorized packages was pulling in a GPL dependency. There is a package that provides the same API but we can't force the dependency on that one

@bolkedebruin bolkedebruin force-pushed the bolkedebruin:AIRFLOW-3697 branch from bce1ba4 to ded53b2 Jan 15, 2019

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 15, 2019

@ashb like this?

@bolkedebruin bolkedebruin force-pushed the bolkedebruin:AIRFLOW-3697 branch from ded53b2 to 09a99cd Jan 15, 2019

@ashb

This comment has been minimized.

Copy link
Member

ashb commented Jan 15, 2019

Yes, thanks.

Though with Slugify 2.0 and #4506 do we still need to vendor this?

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 15, 2019

Yes as it doesn’t work. It’s a setuptools issue

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 15, 2019

@kaxil as you included #4506 in the betas this needs to go in too otherwise we would get the gpl dep.

@kaxil

This comment has been minimized.

Copy link
Contributor

kaxil commented Jan 15, 2019

@bolkedebruin I removed #4506 (revert the commit & rebased) so won't be an issue. Do you think it
would take long to solve this?

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 15, 2019

Haha who knows ✌🏾 Only two left two fix... or hopefully they pass then you can include both again @kaxil

@ashb

This comment has been minimized.

Copy link
Member

ashb commented Jan 15, 2019

Ohh. if the non-GPL module is already installed when do pip install python-slugify it would use that, otherwise it will pick up the GPL one. Right. :(

@kaxil

This comment has been minimized.

Copy link
Contributor

kaxil commented Jan 16, 2019

I will cut RCs tomorrow without this PR as this PR even if completed would just work for rbac FAB UI as we have removed the old UI.

bolkedebruin added some commits Jan 15, 2019

[AIRFLOW-3697] Vendorize nvd3 and slugify
nvd3 has a dependency on python-slugify which pulls in a
GPL dependency by default.

@bolkedebruin bolkedebruin force-pushed the bolkedebruin:AIRFLOW-3697 branch from 09a99cd to 2673481 Jan 16, 2019

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 16, 2019

@Fokko @dimberman @ashb I need a bit of help: I don't understand why this fails on the kubernetes tests

@dimberman

This comment has been minimized.

Copy link
Contributor

dimberman commented Jan 16, 2019

@bolkedebruin looking

@bolkedebruin

This comment has been minimized.

Copy link
Contributor Author

bolkedebruin commented Jan 16, 2019

Thanks @dimberman

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment