Hi Mads, On Sat, Sep 08, 2018 at 08:39:12PM +0200, Mads Kiilerich wrote: > Thanks. I really appreciate having a second opinion and different take > on this. That makes it easier to discuss ;-) > > My questions are mainly about verifying that this "MVP" is a stable > foundation and establishing a shared vision of how this can work for the > next steps we know are coming.
Sure, no issue. > > > # HG changeset patch > > # User Mads Kiilerich <m...@kiilerich.com> > > This patch seems to only reuse a few trivial lines from me. There is no > need to give me credit for this. Doesn't matter to me either. I just started from your original patch. > > > The approach implemented by this commit is to add a custom gearbox command > > 'setup-frontend' to run the required commands. This single user-facing > > command can internally run various steps as needed. The only current > > requirement is the presence of npm and an internet connection. > > What is this command really about, and what is the scope of it? I guess > it essentially is something that prepare all the static files that can > be served directly by the web server ... and perhaps even be served by a > front-end server or put on a CDN? If I take a step back, actually I would like the installation/setup of Kallithea to be as easy as possible for the user. As such, the amount of commands should be kept reasonable, and if it is not necessary to expose certain details to the user (for example because there is no element of choice for it) then it should rather be kept internal and run automatically in some way. Looking at the installation instructions from: https://kallithea.readthedocs.io/en/default/installation.html combined with required steps from: https://kallithea.readthedocs.io/en/default/setup.html#setup we get: 1. hg clone https://kallithea-scm.org/repos/kallithea 2. cd kallithea 3. virtualenv ../kallithea-venv 4. . ../kallithea-venv/bin/activate 5. pip install --upgrade pip setuptools 6. pip install --upgrade -e . 7. python2 setup.py compile_catalog # for translation of the UI 8. gearbox make-config my.ini 9. gearbox setup-db -c my.ini 10. npm install 11. npm run less Steps 1-5 cannot really be changed (also because virtualenv is not a requirement). Step 6 may be fine to expose to the user. Step 7 is definitely something internal IMHO. Step 8-9 need to be exposed because users may want to create different configs, skip setting up the db if they already have one, etc. Step 10-11 are more something internal. So, ideally I would like one command to group steps 7, 10 and 11 and possible future steps needed to set up Kallithea. I'm not experienced in deployment scenarios where static files are put in other places, so can't really comment on that. > > Instead of augmenting the source directory, it should perhaps fully > generate a target directory from scratch, populate it with npm output > and copying additional files from the source tree. Neither Kallithea > source directory nor npm installed stuff should be directly exposed by > the web server. > > It should probably be possible to give the directory to the gearbox > command (and/or configure it in the app config) so it doesn't have to > clobber the site-packages directory but can be installed in a different > place with different permissions and security posture. Which gearbox command are you referring to here? 'serve' or 'setup-frontend' ? To explore this idea further (and to help my understanding), let's consider the different ways to install and serve Kallithea. Please correct me where I'm wrong. 1. Kallithea installed from source repo Here the application is served directly from the repository, the python dependencies are installed elsewhere (either in a virtualenv or from a location in e.g. /usr). the css and js of Kallithea itself is served from kallithea/public/ in the repo, the css from npm dependencies are all accumulated in the big single css file (via npm run less), the js from npm dependencies are currently not used (e.g. bootstrap.js is still coming from the repo itself). 2. Kallithea installed via pip Here the application files are in the virtualenv or in /usr, the css and js of Kallithea itself is served from kallithea/public/ from that location in site-packages, the css from npm dependencies are all accumulated in the big single css file (via npm run less), the js from npm dependencies are currently not used (e.g. bootstrap.js is still coming from the repo itself). Are there other ways? Whether you serve via gearbox, apache, uwsgi, etc. should not matter here, right? So what you want to achieve with this suggestion, is to move the kallithea/public directory to another place, chosen by the user (perhaps by default inside the same directory where their ini file will be and the data directory is?). And in case any other than css/less files from npm are used, make sure they are copied to that public directory too? > > > Is "setup front-end" descriptive for what the command does? Is it really > a "setup step"? Or is the role more similar to "make-config", and could > be named "make-frontend"? Current 'make' and 'setup' commands known to gearbox are: make-config Kallithea: Create a new config file make-index Kallithea: Create or update full text search index make-rcext Kallithea: Write template file for extending Kallithea in Python makepackage Creates a basic python package setup-app Setup an application, given a config file setup-db Kallithea: Configure the database specified in the .ini file I consider 'setup' something more generic than 'make' in the sense that the word 'setup' encompasses any step needed to get something ready, while 'make' refers only to generation of something new. In particular given the comments earlier in this mail about also grouping other stuff like compile_catalog, I would argue that 'setup' is better suited than 'make', although the suffix 'front-end' becomes too restrictive. In fact, in the list is 'setup-app' which sounds suitable to me and as far as I understand is not actually implemented yet. There is a default implementation in gearbox itself but can probably be overwritten. > > I think there are valid use cases for separating the on-line "download > and install npm stuff" part from an off-line "build from source and > downloads" parts. Perhaps give the gearbox command options for just > running one of these parts? I guess the on-line part is setup-ish, while > the off-line build part is more make-ish ... Yes, the download part may need more user/environment specific config like a proxy or other settings for npm. > > I wonder about the omission of '-' in 'front-end' in the command name > that already contains another '-'. Which of the spellings would be least > confusing? > > Perhaps name the command "front-end" (with "--download"/"--install" and > "--build" and "--path=" options)? Based on the comments above I would now actually not call it anything 'frontend' anymore. But you're right that the prevailing spelling seems to be front-end and this should be reflected in the command name. > > > For now, this will just create/update style.css ... but currently probably > > without any actual changes. > > How will this gearbox approach work with other use cases as outlined on > https://kallithea-scm.org/repos/kallithea/pull-request/137/_/remaining_bootstrap_related_stuff_v3#C--b9cfc7f2cdf7 > > : Generated files (pygmentize) and automatic rebuild (kind of as server > --reload) and minification and non-minified development mode (if > relevant?). And JavaScript handling? Can we have a PoC that show how > that could work? pygmentize would be just an additional command to run during the 'setup-app' or whatever we call it, right? For automatic rebuild: not sure. I had given in a comment on that PR a PoC to watch .less and .css files from '--reload' but as Dominik pointed out it does not cover the actual rebuild. So with this mechanism we could let the serve reload when the css is regenerated (which is not actually needed), but the generation itself would need to be triggered manually after the less is changed. Alternatively we use something like nodemon to watch the less files and generated the css, as originally proposed by Dominik. For minification/no minification: I'm still not convinced that we need a choice here. I'd let the command generate both minified and unminified, and let the application use the minified one. Dominik said there may be problems when debugging, but I'd argue that this would be a bug in the debugger, not a fundamental problem with the approach. For JavaScript: I guess the idea is to copy e.g. bootstrap.min.js from node_modules to the public directory instead of shipping it ourselves, and similar for other libraries. Also this would just be another step taken by setup-app, right? Do you see more complexity? > > I don't think I would like to have the application run this > automatically at run-time, but could it be feasible to let the > application fail to start if front-end code hasn't been generated or is > outdated? (That kind of goes in the opposite direction of allowing it to > be hosted elsewhere ... but that could be a special case.) Failing to start when the front-end code is not generated at all seems feasible without much issue: basically we can check the presence of the files referred to in the root.html/base.html. Checking whether it's up to date is more complex. Checking that the kallithea .css is generated from the current .less file may be achievable. For example by saving the md5sum of the .less file into the .css (I didn't check yet if that is possible via less) and checking that when starting the application. But to check whether all other dependencies are fine may be more complex. As we don't yet have such cases I find it hard to reason about it. > > > +class Command(BasePasterCommand): > > + """Kallithea: setup the front-end (JS and CSS)""" > > So far it only does css - no js. But that should hopefully change soon. When I wrote this I did not yet realize this... > > > + takes_config_file = False > > + requires_db_session = False > > + > > + def take_action(self, args): > > + rootdir = > > os.path.dirname(os.path.dirname(os.path.abspath(kallithea.__file__))) > > + print "Running 'npm install' to install frontend dependencies from > > package.json\n" > > We should be consistent in the spelling of front-end (when possible - > identifiers and module names might have to be different). True, that is a mistake. Thanks, Thomas _______________________________________________ kallithea-general mailing list kallithea-general@sfconservancy.org https://lists.sfconservancy.org/mailman/listinfo/kallithea-general
