Bug 1000735

Summary: tinymce should use the new Naming guidelines and install to /usr/share/javascript
Product: [Fedora] Fedora Reporter: T.C. Hollingsworth <tchollingsworth>
Component: tinymceAssignee: Kevin Fenzi <kevin>
Status: CLOSED EOL QA Contact: Fedora Extras Quality Assurance <extras-qa>
Severity: unspecified Docs Contact:
Priority: unspecified    
Version: 22CC: awilliam, kevin
Target Milestone: ---   
Target Release: ---   
Hardware: Unspecified   
OS: Unspecified   
Whiteboard:
Fixed In Version: Doc Type: Bug Fix
Doc Text:
Story Points: ---
Clone Of: Environment:
Last Closed: 2016-07-19 10:18:43 UTC Type: Bug
Regression: --- Mount Type: ---
Documentation: --- CRM:
Verified Versions: Category: ---
oVirt Team: --- RHEL 7.3 requirements from Atomic Host:
Cloudforms Team: --- Target Upstream Version:
Embargoed:
Bug Depends On:    
Bug Blocks: 1000644    

Description T.C. Hollingsworth 2013-08-24 19:05:53 UTC
If we're going to unbundle tinymce from stuff, to save us some pain later we should go ahead and move it to /usr/share/javascript.  I can push patches for all this if everyone's okay with it, but I'd like to clear up a few things first.

The new guidelines also call for "js-" prefixed names.  Since we're going to want to port to TinyMCE 4 at some point (maybe F21), I'd suggest leaving the SRPM as it is and shipping a js-tinymce subpackage from it, since TinyMCE 4 will probably end up shipping js-tinymce and nodejs-tinymce packages from the same SRPM.

I noticed this ships tinymce in /usr/share/tinymce/jscripts/tiny_mce.  Any reason that can't just be /usr/share/javascript/tiny_mce or does something depend on the "jscripts" thingy?

Also, it would be nice if we could just port askbot and the plugin that depends on this at the same time and not mess around with providing a compat symlink and all the rpm bs that entails if at all possible.

Comment 1 Adam Williamson 2013-08-24 19:09:59 UTC
have the new guidelines actually been approved at this point?

Comment 2 T.C. Hollingsworth 2013-08-24 19:11:49 UTC
Yes, they just haven't been officially announced yet:
https://fedorahosted.org/fpc/ticket/323

Comment 3 Adam Williamson 2014-01-14 22:21:09 UTC
Reading the guidelines, I think there's rather a lot more work to do here for full compliance :/

The current package essentially ships a 'compiled' tinymce. tiny_mce.js is concatenated and minified from a whole bunch of source files.

You can look at all the stuff that goes into 'building' a tinymce distribution tarball at https://github.com/tinymce/tinymce/blob/3.x/build.xml . Per the guidelines, I think we're supposed to do at least the concatentation / minification ourselves as part of package build, yes?

If a partial conversion to the new policies is helpful, I can at least adjust it to ship in /usr/share/javascript/tiny_mce . The directory nesting isn't really necessary, it's just that upstream distributes the tarball that way - it contains:

tinymce/
  examples/
    (lots of examples, subdirs)
  jscripts/
    tiny_mce/
  changelog.txt

I think we could ship changelog.txt and examples/ as documentation, and just ship the tinymce/jscripts/tiny_mce/ in /usr/share/javascript/tiny_mce .

Comment 4 Adam Williamson 2014-01-15 00:56:51 UTC
OK, so I've been looking into altering the deps too. The plugin is easy - it just installs files into the tinymce plugin directory - but askbot is a bit trickier.

AFAICS, how askbot uses tinymce is that it calls python-django-tinymce, but overrides a bunch of its default configuration. In Fedora's askbot package's config file, the following are set (mostly based on upstream's sample config):

TINYMCE_COMPRESSOR = True
TINYMCE_SPELLCHECKER = False
TINYMCE_JS_ROOT = os.path.join(STATIC_ROOT, 'default/media/js/tinymce/')
TINYMCE_URL = STATIC_URL + 'default/media/js/tinymce/'
TINYMCE_DEFAULT_CONFIG = {
    'plugins': 'askbot_imageuploader,askbot_attachment',
    'convert_urls': False,
    'theme': 'advanced',
    'content_css': STATIC_URL + 'default/media/style/tinymce/content.css',
    'force_br_newlines': True,
    'force_p_newlines': False,
    'forced_root_block': '',
    'mode' : 'textareas',
    'oninit': "function(){ tinyMCE.activeEditor.setContent(askbot['data']['editorContent'] || ''); }",
    'plugins': 'askbot_imageuploader,askbot_attachment',
    'theme_advanced_toolbar_location' : 'top',
    'theme_advanced_toolbar_align': 'left',
    'theme_advanced_buttons1': 'bold,italic,underline,|,bullist,numlist,|,undo,redo,|,link,unlink,askbot_imageuploader,askbot_attachment',
    'theme_advanced_buttons2': '',
    'theme_advanced_buttons3' : '',
    'theme_advanced_path': False,
    'theme_advanced_resizing': True,
    'theme_advanced_resize_horizontal': False,
    'theme_advanced_statusbar_location': 'bottom',
    'width': '723',
    'height': '250'
}

I don't think they ever really checked this very carefully, because TINYMCE_URL does not exist. python-django-tinymce expects you to set TINYMCE_JS_URL and/or TINYMCE_JS_ROOT . If I'm reading the python-django-tinymce source correctly, if you turn on the 'compressor' function of p-d-t, TINYMCE_JS_URL doesn't matter, so that's why they've never noticed this. Basically, if the compressor is on, all we have to worry about is TINYMCE_JS_ROOT .

So I think what we need to do for askbot is:

TINYMCE_JS_ROOT = '/usr/share/javascript/tiny_mce'

We can drop TINYMCE_URL entirely. The content_css line doesn't need to change, we want to use askbot's 'variant' css there, not unbundle it. Then we should patch the silly tests for the values of TINYMCE_JS_URL and TINYMCE_JS_ROOT out of askbot/startup_procedures.py , and I think that's all it needs.

CCing nirik for a sanity check of this (as he's somewhat familiar with askbot). Nirik, T.C., if you can double-check my work here that'd be great - let me know if it sounds OK, and I'll go ahead and do it.

Comment 5 Adam Williamson 2014-01-15 01:00:48 UTC
oh, and  we should drop the "'plugins': 'askbot_imageuploader,askbot_attachment',"

AFAICS the sole reason askbot forked those is to change their button art. I mean...really.

Comment 6 Adam Williamson 2014-01-15 08:19:51 UTC
OK, so here's the skinny!

askbot is the only 'real' thing that depends on tinymce right now. askbot is broken in Rawhide for other reasons. So, the stakes are pretty low.

So I have pushed through a HIGHLY THEORETICAL - i.e., I don't really know if this crap works - set of builds.

http://koji.fedoraproject.org/koji/buildinfo?buildID=491062 - tinymce-3.5.10-1.fc21 - relocates the important bits of tinymce to /usr/share/javascript/tiny_mce , and should comply with the new web assets policy as regards this relocation (though it still violates it in various other ways).

http://koji.fedoraproject.org/koji/taskinfo?taskID=6407337 - python-django-tinymce-1.5.2-1.fc21 - drops one of that package's *two* bundled copies of tinymce (I don't even) and configures it to use the systemwide copy in the normal course of events. At least...I think it does. Like I said, HIGHLY THEORETICAL. About those two copies - there seems to be a Django convention whereby you can denote 'static' files in your django app and there's some kind of mechanism for bundling all the static files a django app needs together in one place, or something. python-django-tinymce has one copy of tinymce for 'non static' cases and one for 'static' cases, near as I can tell, and alternate default configurations for tinymce's location in tinymce/settings.py depending on whether "staticfiles" or "django.contrib.staticfiles" is set in the Django settings that are passed in to python-django-tinymce by whatever uses it. I don't entirely understand this mechanism and whether it makes sense to 'unbundle' the static copy, so for now I left it alone and just unbundled the non-static copy. The static copy of tinymce is still in this python-django-tinymce build, and the default/fallback JS_URL and JS_ROOT settings point to that copy (not the system wide copy) if 'staticfiles' or 'django.contrib.staticfiles' is set.

http://koji.fedoraproject.org/koji/taskinfo?taskID=6407339 - tinymce-spellchecker-2.0.6.1-2.fc21 - installs the spellchecker plugin to the new location instead of the old one. Nice and simple.

http://koji.fedoraproject.org/koji/taskinfo?taskID=6407409 - askbot-0.7.48-12.fc21 - installs askbot's custom tinymce plugins (I changed my mind about them...I don't think they're actually bogus, now) into the systemwide tinymce's plugins directory before dropping its bundled tinymce, drops the two overly restrictive checks for how the path to tinymce is set when calling python-django-tinymce, and simply completely drops both attempts to set a specific tinymce path for p-d-t from the askbot-settings.py SOURCE2 which I believe is our default askbot config: this should cause it to go ahead and use p-d-t's default copy of tinymce, which as noted above, now ought to be the systemwide copy. It still passes in its custom tinymce layout configuration.

So, in theory, all of that should work out nicely, but we all know about theory...I probably messed up somewhere. Please do check my work. Once we have askbot working again in Rawhide, we can see if this actually works properly. If I find the time I'll try unbundling Roundcube's copy of tinymce according to the new policy tomorrow - that one I can actually test, to see if I'm understanding everything right here.

Comment 7 Adam Williamson 2014-01-15 08:21:41 UTC
ah, hell. I just noticed that askbot-settings.py specifies:

    'django.contrib.staticfiles',

so I think it's triggering the staticfiles crap in p-d-t that I was hoping to avoid having to understand. I'll take a stiff drink and attack that one tomorrow, unless someone else knows about it already and wants to explain it to me.

Comment 8 Adam Williamson 2014-01-15 19:30:10 UTC
I've concluded for now that askbot is completely fucking insane and very badly broken on Rawhide. I'm going to try and understand how p-d-t expects django's staticfiles inclusion stuff to be useful, try and determine whether it makes sense to replace its 'static' copy of tinymce, and then forget about askbot.

Comment 9 Adam Williamson 2014-01-15 20:32:53 UTC
OK, I'm getting a handle on it.

So, django has this whole 'staticfiles' mechanism which, near as I can tell, is intended to gather up all the css and javascript and similar junk from a django project's dependencies and dump copies of them all into a single location, STATIC_ROOT, from which the project can then serve them out.

by default, when you kick this thing off, it will take everything from the static/ subdirectories of the apps it uses.

So basically, if you have a django project that's using the python-django-tinymce app and the django.contrib.staticfiles app, what's ultimately usually expected to be the case is that a copy of tinymce will wind up in the STATIC_ROOT of the django project somehow or other, and that copy of tinymce will be used.

If the project doesn't do anything clever (which askbot does, but never mind) then the 'python django-admin.py collectstatic' run will copy everything from python_sitedir/tinymce/static into the django project's STATIC_ROOT . The only thing in there is a copy of tinymce in a directory called tiny_mce, so you'd wind up with:

STATIC_ROOT/tiny_mce

(and all the static files from any other django apps used by the project in there too, but ignore that for now).

The default upstream config of python-django-tinymce basically says 'if we're using this staticfiles mechanism, then the default location of tinymce is the django project's STATIC_ROOT/tiny_mce' .

So, there's two different moving parts to consider here.

1. Do we want 'python django-admin.py collectstatic' to work for people using our packaged p-d-t? If so, we either need to keep bundling a copy of tinymce in the static/ dir, or set up some kind of mechanism by which the *system copy* of tinymce would be pulled into the project's STATIC_ROOT when a 'collectstatic' run happened. The 'simple' way to do this is just to replace the static/tiny_mce copy of tinymce with a symlink to the system copy. I think it should also be possible to take a slightly more sophisticated approach by setting a default STATICFILES_DIRS setting for python-django-tinymce which points to the location of the system copy of tinymce.

2. Assuming we do keep 'collectstatic' working, when the django staticfiles mechanism is in use for a django project, do we want our packaged python-django-tinymce to default to using the copy that was 'collected' into the project's STATIC_ROOT (however we ultimately wind up actually having that copy of tinymce sourced), or do we want it to default to using the system-wide copy?

I guess my inclination for 1. would be to try and set up the STATICFILES_DIRS mechanism for making 'collectstatic' work. For 2. I'm a little unsure what the 'correct' approach would be. Any thoughts?

Comment 10 Adam Williamson 2014-01-15 20:45:56 UTC
BTW, askbot does something pretty stupid here, in case anyone's trying to follow the whole story: in its default settings, it does this:

PROJECT_ROOT = os.path.dirname(__file__)
STATIC_ROOT = os.path.join(PROJECT_ROOT, 'static')#path to files collected by collectstatic

...

STATICFILES_DIRS = (
    ('default/media', os.path.join(ASKBOT_ROOT, 'media'))

...

TINYMCE_JS_ROOT = os.path.join(STATIC_ROOT, 'default/media/js/tinymce/')

but it doesn't do anything to prevent the collectstatic run from include p-d-t's copy of tinymce as well. If you trace all this out, that means that when you run collectstatic for askbot in its default config, the following happens:

* p-d-t's 'static' bundled copy of tinymce is copied from /usr/lib/python2.6/site-packages/tinymce/static/tiny_mce/ to /usr/lib/python2.6/site-packages/askbot/static/tiny_mce/

* askbot's own bundled copy of tinymce is copied from /usr/lib/python2.6/site-packages/askbot/media/js/tinymce/ to /usr/lib/python2.6/site-packages/askbot/static/default/media/js/tinymce/

* askbot *ultimately* winds up using the copy in /usr/lib/python2.6/site-packages/askbot/static/default/media/js/tinymce/ (that's what the definition of TINYMCE_JS_ROOT accomplishes)

So...that's just awesome design. askbot ships with a copy of tinymce, copies it from one part of its tree to another when you run collectstatic, incidentally pulls in another copy from python-django-tinymce which it does nothing at all with, and then uses the copy it copied from its own source. A perfectly 'stock' deployment of askbot has three copies of tinymce. I don't know whether to laugh or cry.

Comment 11 Adam Williamson 2014-01-16 01:20:10 UTC
So I poked around some more, and I think I at least have a conclusion as to what we ought to do, now.

Basically, I think we need to replace p-d-t's static/ directory contents with symlinks. It looks like the django staticfiles mechanisms really don't provide any kind of mechanism by which an app can declare that its static files should be collected from some specific location other than app/static . At the *project* level you have all sorts of options for overriding where static files should be copied from and to and stuff, but that's how the whole thing is set up: you control stuff at the project level. We just don't have the mechanism to make our packaged django *apps* say 'my static files are not where you're expecting, they're over here'.

So: we're gonna have to use a symlink, unfortunately. Sadly, I think this means we're stuck with the infamous 'can't turn a directory into a symlink or vice versa' problem in RPM, and now we have to go look at all that crud about %pre scripts written in lua, and I'm just about losing the will to live.

But basically what I'd want to do is make python-django-tinymce's static/tiny_mce directory into a symlink to /usr/share/javascript/tiny_mce , and otherwise leave things as they are now. That would accomplish the behaviour we'd want at the app level: if something used python-django-tinymce without using the staticfiles app then it'd serve the systemwide copy of tinymce; if something used python-django-tinymce with the staticfiles app, then when they run 'collectstatic' the system copy of tinymce as it exists at the time they run collectfiles will be copied into their project's STATIC_ROOT. If I haven't misunderstood, that (IMHO) is the 'right' thing to do, insofar as there is one.

god, I hate webapps.

Comment 12 Adam Williamson 2014-01-16 01:27:17 UTC
when it comes to packaging webapps that are basically django *projects*, like askbot is, I can see a theoretically interesting approach where we make sure we have *all* of their "static" dependencies unbundled, and then we define their STATIC_URL as /_sysassets , or something. But the split between _webassetdir and _jsdir would, I think, cause some problems with that; django makes a very strong assumption that all 'static' files for a project should share a common root, it would expect javascript and css deps to be in /_sysassets/(libname) , it wouldn't find ones which were in /_sysassets/javascript/(libname), I don't think. :/

Comment 13 Matthias Runge 2014-01-16 07:20:54 UTC
I'm tending to treat the "/static" dir of a django app as generated junk. Safest bet is to allow copies here (since you can not know, if the web server allows following symlinks)

If askbot brings its own copy of tinymce, it should be possible to drop the dependency to python-djangp-tinymce (or to remove tinymce from askbot).

As far as I understand it here, p-d-t is a very simple app with the only reason to carry tinymce into django projects.

Comment 14 Adam Williamson 2014-01-16 07:31:50 UTC
"As far as I understand it here, p-d-t is a very simple app with the only reason to carry tinymce into django projects."

Not quite - it actually includes a 'compressor' which is an addition, and askbot uses that. Basically it reads all the .js files out of a copy of tinymce, cats them together, and serves them out after gzip compressing them. The tinymce project provides a similar compressor implementation in PHP, but we don't package that, and askbot is pretty innately expecting to consume tinymce via p-d-t, which is quite different to consuming it directly.

It's trivial to unbundle things at what django would call the 'project' level and not 'worry' about doing it at the 'app' level, but we probably ought to be coming up with a consistent policy about what we want to be doing at *each* level.

If you look at the overall logic of how something like askbot works - where it deeply relies on this whole django concept of 'frameworks' containing 'apps' - it's not really easy to 'unwrap' something like django-tinymce. It would certainly be a change that wouldn't be upstreamable and would break frequently.

But I mean, this is hardly the worst problem with askbot. It's really comprehensively broken as a Fedora package as things stand. This isn't quite the bug for that, though.

Comment 15 Adam Williamson 2014-01-16 07:51:35 UTC
oh, and um, in the scheme I laid out above, no web server (except possibly django's built-in server which is only meant for development, not production) would actually be following the symlink from (python_sitedir)/tinymce/static/tiny_mce to /usr/share/javascript/tiny_mce when serving out askbot , that's not how it works.

how it would work in that setup is that when you run 'collectstatic' for askbot (or whatever other django project you have that uses p-d-t), the *collectstatic run* would be the thing that followed the symlink. it'd copy (python_sitedir)/tinymce/static/tiny_mce out to the static/ directory of whatever project you were 'collectstatic'ing. At that point you'd have a real, static copy of tinymce in your project's static/ directory, just like it expected - but it'd be a copy of Fedora's systemwide tinymce, not a copy of a copy of tinymce bundled in p-d-t.

We would leave p-d-t's default TINYMCE_JS_ROOT and TINYMCE_JS_URL settings as they currently are, which would have the effect that django projects which used the 'staticfiles' mechanism and used p-d-t would wind up serving out their static copy of tinymce which was brought in by the 'collectstatic' run. They wouldn't be serving out the systemwide copy.

Comment 16 T.C. Hollingsworth 2014-01-21 06:37:27 UTC
Thanks for working on this, Adam.

AIUI, the "collectstatic" mechanism is the default method of serving static files for production deployments.  The dynamic serving thing is only for the development web server and is described as a potential security risk in the django documentation, so it's something we want to make work but we can't just use it in packaged django applications.

So, we know we want to use collectstatic, and we know how to make django slurp up the system copy when we do that (just use a symlink and curse RPM's poor handling of them ;-), so the only open question I have here is how to go about running collectstatic.

Looking at askbot.spec, there's a commented out call to collectstatic in %post.  Why exactly?  Are all users of django apps supposed to run it manually?  If that's really the case we could just handwave it away and document that folks should rerun it whenever a JS lib updates.  But I'm pretty sure it's only commented out due to some weirdness in Fedora Infrastructure, and we really want to run it in %post.

And that's fine.  I'd just say run it in %post, and also have all django apps add %triggers for each JS lib they depend on that also run collectstatic.  That way, any time tinymce updates, rpm would automatically invoke collectstatic and slurp up the updated system copy.

Make sense?  Is there something I'm missing?

Comment 17 Adam Williamson 2014-01-21 06:54:57 UTC
tc: I honestly kind of stopped caring about askbot after poking at it for a while, I find the whole setup ridiculous and fundamentally hostile to distro packaging and I don't have any particular interest in running it anyway, so it was hard to keep pretending to care. (Also, askbot has requirements for very outdated versions of a whole bunch of the packages it depends on, it looks about impossible to actually make it run in any recent Fedora anyway, it's only really viable on EL6).

if I actually cared about askbot my preference would be to try and finesse the packaging somehow so that it would run 'out of the RPM' as a properly behaved distro package using shared resources, not needing to use the 'collectstatic' crap at all. I believe that would be theoretically possible, but an awful lot of work; roughly it'd involve packaging all the static deps in some kind of consistent way, then fiddling askbot's STATIC_ROOT (or STATIC_DIR or STATIC_PATH or whatever, I forgot) definition. (This is all super hand-wavey, of course). I got the feeling it'd be theoretically possible to set up some kind of policy for django packaging that'd enable us to do this kind of thing, but I know I'm not going to waste my time on it, and I don't know if anyone else wants to.

Matthias and I were in favour of just killing it in Fedora, actually. We strongly suspect it's only *ever* worked in EL6.

Comment 18 Jaroslav Reznik 2015-03-03 15:00:28 UTC
This bug appears to have been reported against 'rawhide' during the Fedora 22 development cycle.
Changing version to '22'.

More information and reason for this action is here:
https://fedoraproject.org/wiki/Fedora_Program_Management/HouseKeeping/Fedora22

Comment 19 Fedora Admin XMLRPC Client 2015-09-20 16:57:02 UTC
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Comment 20 Fedora Admin XMLRPC Client 2015-09-22 20:55:42 UTC
This package has changed ownership in the Fedora Package Database.  Reassigning to the new owner of this component.

Comment 21 Fedora End Of Life 2016-07-19 10:18:43 UTC
Fedora 22 changed to end-of-life (EOL) status on 2016-07-19. Fedora 22 is
no longer maintained, which means that it will not receive any further
security or bug fix updates. As a result we are closing this bug.

If you can reproduce this bug against a currently maintained version of
Fedora please feel free to reopen this bug against that version. If you
are unable to reopen this bug, please file a new report against the
current release. If you experience problems, please add a comment to this
bug.

Thank you for reporting this bug and we are sorry it could not be fixed.