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.

Sign up for GitHub

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

Jump to bottom

Patch for CodeMirror 5.22 #2068

Merged
merged 1 commit into from
Jan 25, 2017
Merged

Patch for CodeMirror 5.22 #2068

merged 1 commit into from
Jan 25, 2017

Conversation

gnestor
Copy link
Contributor

@gnestor gnestor commented Jan 24, 2017 •
edited
Loading

Closes #1967

Continuation of #2066

@gnestor
Copy link
Contributor Author

gnestor commented Jan 24, 2017

@minrk Here is a patch for CodeMirror that resolves #1967.

Build is currently failing with:

Failed to build notebook { Error: Error: ENOENT: no such file or directory, open 'C:\Users\appveyor\AppData\Local\Temp\1\pip-urfuhhtv-build\notebook\static\components\codemirror\mode\meta.js'

I assume this has something to do with patching CM but not sure what...

@gnestor gnestor mentioned this pull request Jan 24, 2017
Merged
1 task
@gnestor
Copy link
Contributor Author

gnestor commented Jan 25, 2017

I took a stab at patching in place by copying a patched codemirror.js from the scripts directory to the bower directory after bower install in setupbase.py. I'm not sure if this is the best way to do it, but builds are passing now! Can you review?

minrk
minrk reviewed Jan 25, 2017
View reviewed changes
setupbase.py
"""Patch CodeMirror until https://github.com/codemirror/CodeMirror/issues/4454 is resolved"""

try:
run(['cp', 'scripts/codemirror.js', 'notebook/static/components/codemirror/lib'], cwd=repo_root)
Copy link
Member

Choose a reason for hiding this comment

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

Rather that run(['cp', it might be better to use shutil.copy, which is probably safer.

Copy link
Member

Choose a reason for hiding this comment

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

Also tools is perhaps a more appropriate place than 'scripts'. 'scripts' is for entrypoints that we install (like jupyter-notebook etc.).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good stuff, thanks!

@minrk
Copy link
Member

minrk commented Jan 25, 2017

Two minor comments, but 👍

@minrk minrk merged commit 0e38b00 into jupyter:cm-up Jan 25, 2017
@minrk minrk modified the milestones: 5.0, 4.3.2 Jan 25, 2017
@gnestor gnestor deleted the pr/2066 branch January 25, 2017 19:08
@minrk
Copy link
Member

minrk commented Jan 30, 2017

@gnestor want to make 4.3.2 with this?

@gnestor
Copy link
Contributor Author

gnestor commented Jan 30, 2017

Ya let's release 4.3.2 today. I'll create an issue for it and give people a chance to chime in before releasing.

@gnestor
Copy link
Contributor Author

gnestor commented Jan 31, 2017

@meeseeksdev backport to 4.x

@gnestor
Copy link
Contributor Author

gnestor commented Jan 31, 2017

@Carreau Can you grant me @meeseeksdev powers? 🎩

@gnestor gnestor mentioned this pull request Jan 31, 2017
Closed
6 tasks
@Carreau
Copy link
Member

Carreau commented Jan 31, 2017

interesting. You do have the power, let me try. It's probaby crashing.

@meeseeksdev backport to 4.x

@lumberbot-app
Copy link

lumberbot-app bot commented Jan 31, 2017

Oops, something went wrong applying the patch... Please have a look at my logs.

@Carreau
Copy link
Member

Carreau commented Jan 31, 2017

Hum.. I don't know why it did not reply to you, but there is an error in backporting, i'll check.

== All has been fetched correctly
2017-01-31T02:21:23.089015+00:00 app[web.1]: Cherry-picking 0e38b009f5f21ef0a480d2095de11537d9530692
2017-01-31T02:21:23.346335+00:00 app[web.1]:   stderr: 'error: Mainline was specified but commit 0e38b009f5f21ef0a480d2095de11537d9530692 is not a merge.
2017-01-31T02:21:23.346397+00:00 app[web.1]: fatal: cherry-pick failed'

@Carreau
Copy link
Member

Carreau commented Jan 31, 2017

BTW you are white listed.

@Carreau
Copy link
Member

Carreau commented Jan 31, 2017

that is strange it is picking up the wrong commit. It does not find the merge commit, which is supposed to be 393a04d....

@Carreau
Copy link
Member

Carreau commented Jan 31, 2017

Oh... it seem like you want to backport #2066 and not this. Can you try the backport command there ?

@gnestor
Copy link
Contributor Author

gnestor commented Jan 31, 2017

Sure, thanks @Carreau!

@jcb91 jcb91 mentioned this pull request Apr 29, 2017
Merged
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 9, 2021
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@minrk minrk minrk left review comments

Assignees
No one assigned
Labels
status:resolved-locked
Projects
None yet
Milestone
4.3.2
Development

Successfully merging this pull request may close these issues.
3 participants
@gnestor @minrk @Carreau