Skip to content

Download jquery during build. #11246

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

Merged
merged 12 commits into from
Feb 16, 2019
Merged

Conversation

anntzer
Copy link
Contributor

@anntzer anntzer commented May 15, 2018

attn @tacaswell, xref #11199

What works:

  • python setup.py sdist and python setup.py bdist_wheel fetch jquery
    and include it in the sdist/wheel.
  • pip install . fetches jquery and puts it to the correct location.
  • pip install -e . fetches jquery and puts it in the source tree.

Should be Py2-compatible, so backportable to 2.2.4.


(previous notes, now fixed)

What needs to be done (cf TODOs):

  • If there is already a file named jquery-3.3.1.min.js, check its hash
    to decide whether we want to download it again or not.
  • Factor out duplicated code.
  • Make code Py2-compatible.
  • Repeat the same thing for jquery-ui and for non-minified versions.
  • Update the gitignore accordingly.
  • Tests?
  • Update the html templates to use whatever version of jquery we have.

Some additional data points worth considering:
jquery seems alive and well with at least one minor release per year (https://en.wikipedia.org/wiki/JQuery#Release_history). OTOH jquery-ui seems in a not great shape (http://blog.jqueryui.com/2017/12/the-future-of-jquery-ui-and-jquery-mobile/, last release was in September 2016). Dunno anything about js to understand what that implies.

Downloading the latest jquery-ui (1.12.1) from https://jqueryui.com/ gives a zip which also contains a not-so-recent jquery (1.12.4) (but a non-minified one only), dunno enough about js compatibility policies to know what that implies either, but I guess an option would be to download the latest zip of jquery-ui and just get whatever jquery version that gives us.

PR Summary

PR Checklist

  • Has Pytest style unit tests
  • Code is PEP 8 compliant
  • New features are documented, with examples if plot related
  • Documentation is sphinx and numpydoc compliant
  • Added an entry to doc/users/next_whats_new/ if major new feature (follow instructions in README.rst there)
  • Documented in doc/api/api_changes.rst if API changed in a backward-incompatible way

@anntzer anntzer added the Build label May 15, 2018
@anntzer anntzer changed the title Download jquery during build (proof of concept). Download jquery during build (proof of concept, do not merge). May 15, 2018
@jklymak
Copy link
Member

jklymak commented May 16, 2018

This failed most tests on the boilerplate issue. OTOH, it is also failing py 3.5...

@anntzer
Copy link
Contributor Author

anntzer commented May 16, 2018

Rebased and fixed py3.5 issues.

@anntzer anntzer mentioned this pull request May 31, 2018
@tacaswell tacaswell modified the milestones: v2.2.3, v2.2.4 Jul 30, 2018
@anntzer anntzer force-pushed the download-jquery branch 2 times, most recently from dc7bcb1 to 090a743 Compare August 16, 2018 14:07
@anntzer anntzer changed the title Download jquery during build (proof of concept, do not merge). Download jquery during build. Aug 16, 2018
Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

There is currently no way to replace the download by a local version (e.g. via an environment variable as suggested in #11199). I think we should not force an internet connection for builds.

Would it make sense to define a local path within the checkout, e.g. .cache/jquery-ui-1.12.1 as the source? People can then manually copy it there if they don't want a download. Otherwise, we download and extract it there and subsequently use this version.

Further advantage: This also reduces the downloads a bit, because we have just one download per checked out repo, not per install/develop.

# Note: When bumping the jquery-ui version, also update the versions in
# single_figure.html and all_figures.html.
url = "https://jqueryui.com/resources/download/jquery-ui-1.12.1.zip"
if not os.path.exists(os.path.join(dest, "jquery-ui-1.12.1")):
Copy link
Member

Choose a reason for hiding this comment

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

Better not to hard-code the subdirectory. This can easily go wrong in a future update.

targetdir = Path(dest) / Path(url.split('/')[-1]).stem
if not targetdir.exists():

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think forgetting to update the HTML template is a much worse risk. Plus see below re: py2 compatibility.

# single_figure.html and all_figures.html.
url = "https://jqueryui.com/resources/download/jquery-ui-1.12.1.zip"
if not os.path.exists(os.path.join(dest, "jquery-ui-1.12.1")):
try:
Copy link
Member

Choose a reason for hiding this comment

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

os.makedirs(dest, exist_ok=True)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Written to be py2 compatible to simplify the backport (we can always update it later).

@anntzer
Copy link
Contributor Author

anntzer commented Sep 9, 2018

@tacaswell said he would handle the caching :)

Copy link
Member

@timhoffm timhoffm left a comment

Choose a reason for hiding this comment

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

Approved. But should only be merged, when there is also caching in place.

@jklymak jklymak modified the milestones: v2.2.4, v3.1 Sep 10, 2018
@jklymak
Copy link
Member

jklymak commented Sep 28, 2018

I'm confused - should we merge this and close #11768?

@anntzer
Copy link
Contributor Author

anntzer commented Sep 29, 2018

No, @tacaswell wants to do some more work on it first.

@tacaswell tacaswell modified the milestones: v3.1.0, v2.2.4 Feb 4, 2019
@tacaswell tacaswell self-assigned this Feb 4, 2019
@tacaswell
Copy link
Member

@sandrotosi You may need to download jquery-ui as part of the debian build process now (as iirc you run the builds without a network connection).

tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Feb 16, 2019
BLD: devDownload jquery during build

 Conflicts:
	examples/user_interfaces/embedding_webagg_sgskip.py
	lib/matplotlib/backends/web_backend/all_figures.html
	lib/matplotlib/backends/web_backend/single_figure.html
	setup.py
	setupext.py
tacaswell added a commit to tacaswell/matplotlib that referenced this pull request Feb 16, 2019
@tacaswell
Copy link
Member

#13446 and #13445 are the backports.

tacaswell added a commit that referenced this pull request Feb 16, 2019
…-v3.0.x

Merge pull request #11246 from anntzer/download-jquery
@anntzer anntzer deleted the download-jquery branch February 16, 2019 11:11
@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2019

Well, sorry, my py2fu was too rusty :)

sha=sha, file_sha=file_sha, url=url))

try:
write_cache(sha, file_contents)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This writes the tarball to a file named after its sha, not after the "normal" filename, which is not so great if one wants to manually download the file and insert it in the cache for example. Using the "normal" filename (something like jquery-foo.tar.gz) doesn't seem to be problematic, or is it?

Copy link
Member

Choose a reason for hiding this comment

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

I think this is ok, it writes it to it's sha in the cache (and I don't think we want to document and support people writing things to the cache). The directions are about where to un-pack the tarballs into the source tree.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Moving the discussion to #13450.

@sandrotosi
Copy link
Contributor

@tacaswell we are not allowed to download anything during debian package build, so i'm gonna have to skip it and use the package jquery instead (JFYI)

@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2019

Note that you can just put a copy of the jquery tarball of the cache dir and matplotlib will find it there (@tacaswell perhaps this warrants a note in the docs?)

@sandrotosi
Copy link
Contributor

we dont really have access to the jquery tarball; i'll figure out a way (i havent checked the actual code yet)

@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2019

Can't you just say that building matplotlib requires the matplotlib tarball and the jquery tarball? (don't know anything about the debian packaging system but on arch linux that would just mean putting both of them in the $sources array).

@sandrotosi
Copy link
Contributor

the idea is that we prefer not to have multiple copies of the same software, in case a security fix is required (how can you find all the places where that software is?)

@anntzer
Copy link
Contributor Author

anntzer commented Feb 16, 2019

ah, I guess you used to just patch the whole jquery away previously to point to your own version? then you can probably just do the same here?

except Exception as ex:
raise ex
else:
raise IOError("Failed to download FreeType. Please download " +
Copy link
Contributor Author

Choose a reason for hiding this comment

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

@tacaswell I don't think this clause can ever be triggered? In the for loop above, either you immediately break, or you immediately rethrow the exception, in fact I don't think you can even ever try the second url...

Copy link
Member

Choose a reason for hiding this comment

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

yup, that raise should be pass.

@tacaswell
Copy link
Member

@sandrotosi it also depends where you pull our source from. If you pull the sdist from pypi then it will already have jquery vendored.

NelleV added a commit that referenced this pull request Feb 17, 2019
@anntzer anntzer mentioned this pull request Feb 28, 2019
6 tasks
@QuLogic
Copy link
Member

QuLogic commented Mar 3, 2019

You know, I forgot about this before, but there's also the XStatic family of packages we could use: XStatic-jQuery / XStatic-jQuery-UI.

@sandrotosi I made this patch for Fedora.

@anntzer
Copy link
Contributor Author

anntzer commented Mar 3, 2019

that's a cute project...

@zerothi
Copy link
Contributor

zerothi commented Mar 29, 2019

Regarding this I have an issue with downloading jquery-ui :(

I do a regular build and get:

error: Failed to download jquery-ui.  Please download https://jqueryui.com/resources/download/jquery-ui-1.12.1.zip and extract it to build/bdist.linux-x86_64/egg/matplotlib/backends/web_backend/.

Before the build I do:

wget -o jquery-ui-1.12.1.zip <url>
pushd lib/matplotlib/backends/web_backend
unzip <path-to>/jquery-ui-1.12.1.zip
popd
python setup.py build

Which places the sources correctly!

I then get:

error: Failed to download jquery-ui.  Please download https://jqueryui.com/resources/download/jquery-ui-1.12.1.zip and extract it to build/bdist.linux-x86_64/egg/matplotlib/backends/web_backend/.

Note, this works for Py3 and matplotlib 3.0.3, but not for Py2 and matplotlib 2.2.4.

Is there someway of specifying an external file position of the jquery-ui file?
In setup.py I couldn't figure out to bypass the download?

In general I think it would be ill-advised to require a web-connection for installation processes.

@tacaswell
Copy link
Member

@zerothi ,where are you getting the source from? If you pull the tarball from pypi it should have jquery pre-downloaded.

We went back on forth on this a bit and settled on downloading jquery at installation / sdist creation time being the best option to avoid having to continue to vendor jquery-ui.

Very odd that it works for py3 but not py2...

@zerothi
Copy link
Contributor

zerothi commented Apr 1, 2019

I downloaded from github with v being the corresponding version.

https://github.com/matplotlib/matplotlib/archive/v$v.tar.gz

Yes, I am also puzzled by this fact. I had the exact same procedure and also checked hash etc.
I agree it would be nice to not have it included. But some explicit way of specifying the download position seems ideal? I could download from pypi, but would prefer direct sources ;)

@zerothi
Copy link
Contributor

zerothi commented Apr 1, 2019

@tacaswell I now downloaded: https://files.pythonhosted.org/packages/1e/20/2032ad99f0dfe0f60970941af36e8d0942d3713f442bb3df37ac35d67358/matplotlib-2.2.4.tar.gz

and used a custom setup.cfg

# setup.cfg
[directories]
basedirlist = /opt/generic/gen-freetype/2.8.1
[test]
local_freetype = False

and ran:

python setup.py config
python setup.py build
python setup.py install --prefix /path/to/install

And I still fails at install with this:

error: Failed to download jquery-ui.  Please download https://jqueryui.com/resources/download/jquery-ui-1.12.1.zip and extract it to build/bdist.linux-x86_64/egg/matplotlib/backends/web_backend/.

The config step yields:

Edit setup.cfg to change the build options

BUILDING MATPLOTLIB
            matplotlib: yes [2.2.4]
                python: yes [2.7.16 (default, Mar 28 2019, 13:39:37)  [GCC
                        8.3.0]]
              platform: yes [linux2]

REQUIRED DEPENDENCIES AND EXTENSIONS
                 numpy: yes [version 1.16.2]
      install_requires: yes [handled by setuptools]
                libagg: yes [pkg-config information for 'libagg' could not
                        be found. Using local copy.]
              freetype: yes [version 2.8.1]
                   png: yes [version 1.6.36]
                 qhull: yes [pkg-config information for 'libqhull' could not
                        be found. Using local copy.]

OPTIONAL SUBPACKAGES
           sample_data: yes [installing]
              toolkits: yes [installing]
                 tests: no  [skipping due to configuration]
        toolkits_tests: no  [skipping due to configuration]

OPTIONAL BACKEND EXTENSIONS
                macosx: no  [Mac OS-X only]
                qt5agg: yes [installing, Qt: 5.7.1, PyQt: 5.9; PySide2 not
                        found]
                qt4agg: no  [PySide not found; PyQt4 not found]
               gtk3agg: no  [Requires pygobject to be installed.]
             gtk3cairo: no  [Requires cairocffi or pycairo to be installed.]
                gtkagg: no  [Requires pygtk]
                 tkagg: yes [installing; run-time loading from Python Tcl /
                        Tk]
                 wxagg: no  [requires wxPython]
                   gtk: no  [Requires pygtk]
                   agg: yes [installing]
                 cairo: no  [cairocffi or pycairo not found]
             windowing: no  [Microsoft Windows only]

OPTIONAL LATEX DEPENDENCIES
                dvipng: yes [version 1.15]
           ghostscript: yes [version 9.26]
                 latex: yes [version 3.14159265]
               pdftops: yes [version 0.48.0]

OPTIONAL PACKAGE DATA
                  dlls: no  [skipping due to configuration]

running config

@jdemeyer
Copy link

jdemeyer commented Jun 20, 2019

If you pull the tarball from pypi it should have jquery pre-downloaded.

This doesn't actually seem to work: #14585 (to be precise: it does have jquery-ui in the tarball, but it downloads jquery-ui anyway)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants