sandstormports/community-project

Dan: Upgrade Etherpad package (to 1.18.14 or close)

orblivion opened this issue · 87 comments

orblivion commented

Total

(This got a little complicated since there was wrapup stuff not really in any category. Will probably leave milestone metrics below as "out of date" unless people are very interested.)

  • Estimated Originally: 17.5-44.5 hr
  • Currently: 38 hrs at the point of experimental app release. Will update after it's in the market in case I need to fix things.

Milestones (out of date)

This section got out of date as far as hours, and it'll probably take more time than it's worth to get back up to date. But keeping around in old state in case anyone cares.

Get started on spk based on Ian's vagrant-spk version

  • Convert vagrant-spk -> spk
    • Estimated: 2-4 hr
    • Actual: 3 hr

Caveat on vagrant-spk -> spk:

  • Figure out why spk packed version can't find any files in the package.
    • Not /usr/bin/bash, not launcher.sh, nothing.

Based on Kenton's changes:

  • Sandstorm Packaging Stuff

    • Graphics, md files, pkgdef, etc
    • Estimated: 1 hr

  • Trivial items

    • Remove excessive logging and console warnings
    • Estimated: .5 hr

  • Style

    • Style tweaks, some for avoiding cutting things off (avatar in sidebar, which Kenton added to ep_author_neat)
    • Estimated: 1-4 hr
      • Pessimistic end of estimate: the new version could be styled sufficiently differently that these tweaks are nontrivial to replicate.

  • Minifying/Caching/Performance

    • Upstream etherpad (as of the old version, at least) minifies on runtime. Kenton's etherpad minifies during the build phase (need to run etherpad outside of Sandstorm manually to trigger it), saving results to ./cache which gets packaged. At runtime, it uses ./cache and and bypasses all minifying.
    • Estimated: 2-6 hr
      • This seems like something that may or may not have changed a fair amount under the hood over the years.
    • Actual (so far): 10hr

  • Dependencies

    • Install plugins, including two modified plugins (ep_comments_page and ep_author_neat) from source. (these have their own milestones)
    • Install sqlite3 and capnp.
    • Estimated: 1-10 hr
      • Long side of this estimate is based on updated plugins causing surprise problems.
    • Actual: 4 hr

  • Migrating dirty.db -> sqlite

    • Dirty.db is Etherpad's default database. Etherpad-Sandstorm used it for very early versions. We ideally should still migrate for the sake of those folks.
    • Estimated: .5 hr
      • I don't see how changes in the latest Etherpad could create surprises here given that this migration runs in the kickoff script.

  • Sandstorm Profile Integrations

    • Automatically populate author name and avatar. Make avatar available in ep_author_neat (see below).
    • Estimated: 2-4 hr
    • Actual: 4hr

  • Sandstorm Auth/Permission Integrations

    • Replace Etherpad identity and permission with Sandstorm's
    • Estimated: 2-4 hr
      • Would be faster, except I worry there may be new places that I have to patch in this permission. Risk may be unauthorized access.
    • Actual: 3.5hr

  • Sandstorm Activities Integrations

    • Posting Sandstorm activity on edit
    • Estimated: .5 hr
    • Actual: .5hr

  • Assorted

    • Settings - initial messages and other cosmetics, basic configs. One text change (which Kenton has noted may warrant translation). Redirect to a document instead of the Etherpad home page.
    • Estimated: 1-2 hr
      • Gonna check for new settings. Something interesting could come up.

  • ep_author_neat

    • UI/styling bugfixes and tweaks. Alter behavior for attributing a paragraph to an author. Show avatar in the sidebar (using changes in etherpad-lite).
    • Estimated: 2-4 hr
      • Some competing upstream changes
    • Actual: 4 hr

  • ep_comments_page

    • UI/styling bugfixes and tweaks. Sandstorm permission: comment. Sandstorm activity: comment.
    • Estimated: 2-4 hr
      • Upstream made a lot of commits
    • Actual: Way over, but I'll count 4 hr
orblivion commented

Not sure how much community feedback you're looking for first, so lmk when I should go ahead.

I'd start with vagrant-spk -> spk, since testing everything else sort of depends on that.

ocdtrekkie commented

One of the challenges we've encountered through this process is soliciting community feedback. I think if Ian and I are good with it (I am), I think you should start working, as we are the two people currently administrating this funding, and there isn't a conflict of interest (i.e. we are not paying Ian). The terms we are discussing are not dissimilar from what we'd previously discussed if Ian took on the project, so there's been plenty of time for people to raise objections to the rate or the goal.

So perhaps wait for @zenhack to sign off here, and then consider that a go-ahead.

garrison commented

All sounds good to me. I am merely curious if someone could post a few sentences explaining the vagrant-spk -> spk change (motivation, etc.).

zenhack commented

I think the reason for this is that @orblivion uses QuebesOS, which, since it uses virtualization for its own isolation layer, makes running VMs difficult. I think if that initial overhead will make him otherwise able to be more productive it makes sense, though it is important that the build process is well documented and portable.

zenhack commented

Also, I think we should send out a thing to the mailing list and wait a day or two before pulling the trigger.

zenhack commented

(I sent out a thing)

orblivion commented

That's right, I can't really do a VM within a VM. And I just enjoy the simplicity of it. Qubes lets me make a VM to separate everything anyway, I may as well just have simple access to the machine.

That said, since I'll be paid for this, and since others may need to pick up the work after me some day, I'm thinking I should take a minute to look into running Vagrant on QubesOS. Maybe there's a way with some sort of caveat. Perhaps it could use Qubes' VM system, or perhaps it can work as totally emulated (and not unusably slow).

orblivion commented

Yeah, if there's a way it's not very obvious. It doesn't work out of the box at any rate.

zenhack commented

Alright, it's been few days and no one has spoken up; I think we can give you the green light.

orblivion commented

Works with spk in addition to vagrant-spk:

https://github.com/orblivion/etherpad-lite-sandstorm/

Turns out there's not a whole lot different between the two for this package so hopefully vagrant-spk still works just as well. See README.

Includes finishing up an optimization that Ian started (which Kenton didn't even include).

orblivion commented

Oh, big caveat: spk pack produces something where sandstorm can't seem to find any files referenced by the packagedef (???). @zenhack says he will look into it; it's only blocking release, not development. Worst case we can see if someone else can build it.

garrison commented

Great!

Oh, big caveat

Yeah, this sounds surmountable to me.

orblivion commented

Next is installing dependencies. See latest on my repo for the results of that. (4 hours so far.)

I initially included the altered plugins (ep_comments_page and ep_author_neat) in the list for the Dependencies milestone. This was a mistake given that they each have their own milestones.

Just for the sake of sticking to the estimate plan, I tried rolling with it installing them as-is, and they sort of looked promising, but there seem to be upstream changes that are required for the latest etherpad-lite. I'll need to rebase Kenton's changes for them onto latest upstream in their respective milestones.

Assuming people are fine with excluding the altered plugins from the Dependencies miletsone, there's one more thing before I call this done. I'd like some feedback on what I found with the unaltered plugins (see below).

Results

sqlite3

Ian already had taken care of sqlite3.

capnp

I had to wrestle a bit with capnp but I got it to the point where I'm able to require("capnp") without complaints. Confirming that it actually works is part of the "activities" milestone (in retrospect, estimating activities at .5 hours might have been a mistake given that capnp is a newer verison than our previous package, but I could get lucky).

unaltered plugins

They seemed to work fine with the below caveats. I'm curious about your opinions.

Note: Some are New Bugs (only after upgrade) and Existing Bugs (already exist in our previous package). Also, none of these bugs seem to be triggered by being within Sandstorm. They exist when running Etherpad directly.

ep_sticky_attributes

This plugin is supposed to make it so that if you click "bold" (etc) and type, it will type bold. Like a normal word processor. Without this plugin, the only function of the bold button is to format selected text.

Existing Bug: turning on bold, italics etc is indeed sticky, but turning it off is not. (I suppose you could say it's too sticky)

ep_font_family:

Font choice is not "sticky", and ep_sticky_attributes does not change this. You can only select areas and change their font. Though, if you move your caret to the middle of an area that is already a different font, then you can continue typing in that font.

New Bug: The font dropdown menu does not react when you move the caret to an area that no longer has that font.

Granted, in the previous Etherpad package, moving the caret would just switch to "Font Family" (the name of the dropdown) rather than telling you the name of the font at the new location of the caret. But at least it wouldn't be incorrect information.

ep_font_size:

New Bug: Same bug as ep_font_family but for font size.

ep_markdown

Existing Bugs: This plugin is really buggy, at least given the other stew of formatting plugins in the mix. That said, import seems to work better than in the old version. 🤷

Maybe also notable that the markdown format is changed between these two versions.

I vote we nix this plugin. It's a mess. I can't imagine it being useful. Try it out though (on the existing package) to see.

ocdtrekkie commented

Are these bugs reported to the respective Etherpad repos? I guess my hope is that we can retain the features close to the original package, but ideally that Etherpad plugin updates will resolve these issues independently of our more maintainable Etherpad package.

orblivion commented
orblivion commented

BTW I meant "caret" when I said "cursor". I corrected my comment.

The author (or one of them) of the font plugin chimed in and said he fixed the logic around the headings plugin to fix this bug. Indeed the bug does not appear there. So maybe that's an easy fix. I won't have time to look for a bit tho.

zenhack commented

Thanks for the update.

I'm fine with junking the markdown plugin -- indeed, I once started an etherpad doc working under the assumption that I could export it as markdown, and then was annoyed when I found the output was really bad. It would have saved me some time if it was clear that wasn't going to work well at the outset; I would have just used SandMD or something.

Re: node-capnp, I don't expect too much trouble; most of what's changed is just internals to work with newer versions of the v8 FFI.

WRT milestone bookkeeping, entirely up to you how you want to count that; you're not billing us per milestone, so whatever makes it easiest for you to keep track of things is fine with me.

orblivion commented
zenhack commented
orblivion commented

Looking a little more closely at the same question with regard to the font color plugin: in our current Etherpad package, moving the caret around does not change the value of the dropdown box. In the latest version of Etherpad and the plugin, it does, though it has an issue:

If you select an area that's all red, it correctly shows up as "red". If you select an area that starts red and ends green, it correctly shows up as "Font Color", indicating that it can't say one way or the other. If you select an area that starts red, goes to green, and ends back on red, it incorrectly (imho) sets the dropdown to "red".

So, it doesn't look at every character in the selection, only the first and last character. I discovered this while looking at its implementation, while trying to fix the aforemention font size and font family bugs. The facility functions provided by etherpad to get the attributes at the various characters don't appear to be very friendly toward implementing it correctly.

Talking it over with John from Etherpad here: ether/ep_font_family#29

Question: Supposing John wants to bring Font Size and Font Family up to par with Font Color with this regard, do we count this as a regression and stop the presses? Or is this not exactly a bug?

orblivion commented

I also have a worst-case proposal:

Let's say at the end of the day we don't like what the plugins are doing with the dropdowns, or maybe it will be fixed it but it'll just take more time than we want to wait/spend right now. We could fork these plugins and short-circuit them, such that it works like our old etherpad package, in that the dropdown does not reflect the selection at all.

This would be very easy to do.

ocdtrekkie commented

I am not super worried about non-Sandstorm-impacted plugin behavior, as hopefully Etherpad will patch it at some point, and a dependencies-updated release hopefully won't be a big deal for your new package.

If it's not data corrupting, but just behavior weirdness, I wouldn't worry too much about it.

  • 👍3
orblivion commented

Okay. I'll call this milestone done and move on for now then.

zenhack commented

Agreed; I'm not inclined to spend time fussing with minor upstream bugs & regressions, unless they somehow disproportionately affect Sandstorm users.

orblivion commented

I'm working on the caching/minifying milestone now. I just want to give a heads up - I'm closing in on the estimate high end (6 hours) at this point. I feel like I'm getting there, but there seem to be lots of rickety moving parts that keep throwing me surprises.

orblivion commented

@zenhack I found that you have settings.json in rootfs rather than putting it in the etherpad-lite directory with a patch. (And I separately put it in a patch, and am just now discovering it in rootfs). So I should do one or the other. Any reason it ought to stay in rootfs?

zenhack commented
zenhack commented
orblivion commented

At this point I'm at 10 hours (well over estimate) on performance (caching / minification). I'm at a strange point where it looks like it's working from the outside, but when I console.log debug, it behaves in a way that - while repeatable/consistent - doesn't really match Etherpad's comments and/or Kenton's comments. (I started this off by porting Kenton's patch to modern Etherpad). Consulting with @zenhack we're not comfortable shipping it without knowing why it behaves how it does. I'm also not 100% done on minification and prefetching since I focused on caching, but those seem much less complicated to finish.

If we had time to spare and spend, I'd try to understand Etherpad's caching middleware better. And maybe just rewrite it for our needs, since it's rather different from vanilla Etherpad's needs. Per suggestion from @zenhack I will ping Kenton to see if he has a moment to look (if I'm lucky he'll remember something about this particular patch from years ago), and move on to other milestones for now. Hoping I can beat some more worst-case estimates and carve out more time for performance.

orblivion commented

Putting aside caching to make sure the rest of this update can get done in reasonable time. For now it seems that all the caching work hasn't made it any faster than simply turning off minimization. But perhaps we can make something of perf when we revisit later.

Moving on to other things: Basic sandstorm integrations and permissions are done, within estimate. I'll update the estimate numbers in a little bit.

Next, looking at the fork of ep_author_neat. My pre-estimate read was that there were new commits on top of where Kenton's version was forked from. Looking again now, there are no new tags (or npm releases) with those changes, so I'm going to assume those changes aren't release ready (or at least not required). Starting with Kenton's fork as-is, it basically works. The only problem is styling quirks.

Related question: how do we feel about line numbers? They seem to come standard now with etherpad. (The main styling issue with ep_author_neat has to do with the avatar smashing into the numbers.)

orblivion commented

BTW the previous Etherpad package had some code related to the Etherpad API endpoint:

kentonv/etherpad-lite@6150fe7#diff-414c13f51576e12aadeab256bc233c294997b60cd8cdbd9ac07c79208732c4bc

I didn't include it (and just blocked off the API instead) because I'm inclined to think it was never used. But, I want to doublecheck with people here.

My thinking is this: Firstly, we don't have a Sandstorm API endpoint set up in the pkgdef, so we didn't expose it that way. Secondly, it uses the sandstorm permission header, which means the only client could have been the browser. Etherpad tends to use sockets for such things, and the app still works after I block the API, and I didn't see any client code referring to the API "functions" anyway.

ocdtrekkie commented

IMHO, we position our Etherpad app as a Word or Docs competitor, where collaborative document editing is the key feature. I think author bubbles are probably more important than line numbers for our package, if we have to pick one or the other.

Your remarks on the API seem sensible. It'd be nice to get Kenton's thoughts if he remembers why some things were done, but if you don't see a working case for it, shelve it for now.

ocdtrekkie commented

As a side note, I took a brief look at Etherpad's HTTP API (https://etherpad.org/doc/v1.8.14/#index_http-api), in order to answer the question "do we want to implement this"... and I really can't even fathom. It sounds like the capabilities are mostly around managing multiple documents (of which does not apply to Sandstorm), and to a given document, has basically a get/set contents option only. So there probably isn't a rich amount of use to even strive for there.

orblivion commented

Update:

I took some ep_author_neat hours setting up the same git-patch system for it and ep_comments_page. I took up the rest of it fixing avatar size and positioning. I managed to preserve the line numbers. Some CSS hackery (which is not my forte, glad I could figure it out). However, there was also an avatar bug in the integrations, so I spent another hour on that. Both of those milestones are now maxed out.

Brighter news is that ep_comments_page (which I haven't spent any real time on) works pretty well out of the box. And the overhead for the git-patch is already done. Big thing is that there are some permissions issues I have to fix. And who knows if I'll find a bug. I'm not sure if I'll need all of Kenton's fixes since the plugin has changed a fair amount, particularly style-wise.

Also, the "style" milestone may be a nothing. Either not necessary or covered by the plugin work above anyway. However, I wonder about the width of the document in our current version of the Etherpad app vs the width in the version of Etherpad I'm working on now. The current version is thinner, and looks more like Google Docs or what have you. I wonder if that's what we want, or if maybe the old version was also thin and Kenton widened it on purpose? I haven't seen anything in the changelog yet that would suggest this is the case.

Aggregate hours at the bottom should now reflect ep_author_neat, profile integrations, and incomplete over-estimate caching stuff.

orblivion commented

I pushed the change to include ep_comments_page. I grossly underestimated what was involved since I didn't realize during estimation phase that our previous package changed the interface to view comments via click instead of mouseover. Upstream changed enough since then that it took a while to do (on top of other changes I needed to do).

So, I went way over time, but I'm just going to call it 4 hours for our budgeting purposes. EDIT: i.e. I'm only going to charge for 4.

orblivion commented

Updated numbers; I forgot a couple things and/or made some mistakes.

Also, I think Style is going to be 0, but I want to confirm that people think the wider margins (I think they're just from the more recent etherpad) are desired once I figure out the problems building the spk.

orblivion commented
orblivion commented
ocdtrekkie commented

Note that in my example at sandstorm-io/sandstorm#3598 you cannot open old grains with the new app. It displays them in the list for the new app, but since they're shared with me, they open with whatever package the grain owner has set for them.

orblivion commented

Ah yes, same experience here. That's a relief. And I suppose it makes sense; list and open are different functions.

zenhack commented

On my local dev instance, when I start a new grain I get a pop-up that shows:

An error occurred

Please press and hold Ctrl and press F5 to reload this page

If the problem persists, please send this error message to your webmaster:
Error: Ace2Editor.init() error event while waiting for load event
at http://ui-8e62f9f36bc376e2c86c620bd187ad6d.local.sandstorm.io:6080/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=7601783c at line 2994
ErrorId: WRyoo1coVpVsG7bm99fy
Unhandled Promise rejection
URL: http://ui-8e62f9f36bc376e2c86c620bd187ad6d.local.sandstorm.io:6080/p/main
UserAgent: Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101 Firefox/95.0

Behind it is etherpad's loading page. Refreshing results in the same error. This shows in the grains' debug console:

[2022-01-07 17:35:34.678] [WARN] client - Error: Ace2Editor.init() error event while waiting for load event -- {
  errorId: 'WRyoo1coVpVsG7bm99fy',
  type: 'Unhandled Promise rejection',
  msg: 'Error: Ace2Editor.init() error event while waiting for load event',
  url: 'http://ui-8e62f9f36bc376e2c86c620bd187ad6d.local.sandstorm.io:6080/p/main',
  source: 'http://ui-8e62f9f36bc376e2c86c620bd187ad6d.local.sandstorm.io:6080/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=7601783c',
  linenumber: 2994,
  userAgent: 'Mozilla/5.0 (X11; Linux x86_64; rv:95.0) Gecko/20100101 Firefox/95.0',
  stack: 'errorCb@http://ui-8e62f9f36bc376e2c86c620bd187ad6d.local.sandstorm.io:6080/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=7601783c:2994:19\n'
}

It seems to work fine on alpha though...

zenhack commented

Re: notes:

  • I agree with keeping the new theme.
  • I agree with keeping the new logo.
  • Startup time does seem within reason (on alpha anyway; on my local dev instance it never really gets there, see above), though if you think you can improve it further that would be great. Though we should def. get everything working before spending a bunch of time on this I think.
ocdtrekkie commented

I need to play a bit more this weekend, but it seemed much less quirky than our old Etherpad package at first glance. It just feels like we've moved to a modern document editor for the first time, so I'm pretty excited.

Performance seems okay, but single document grains are always best if they start super fast. Perhaps once upgrade is smoothed out a bit, we can ask Kenton to review (since he will be involved in either the key rotation or transfer), and maybe he may have some suggestions or insight on startup improvements.

orblivion commented
  • 👍1
orblivion commented

I just pushed a fix to the package to get it to build and run with vagrant-spk. This was the biggest problem that I had to work around:

sandstorm-io/vagrant-spk#320

Check out build.sh for more.

While I have gotten spk pack to work (though I'm still figuring out details with sandstorm-files.list), vagrant-spk pack doesn't seem to work just now. Maybe for trivial reasons, I only spent a minute on it. I don't know that I'll have time to fix it myself. I came into this project with the understanding that I could do it with spk. At this point I have my other computer set up I could do either but if my time is limited I'll do whichever is easiest.

ocdtrekkie commented

I came into this project with the understanding that I could do it with spk.

I am absolutely good with it if it only works in spk, though if it works in both, it'd be a relatively nice perk.

I think really the only thing missing for publishing is making sure the upgrade works from the old Etherpad, and then seeing if we can get Kenton to review it, and decide how to hand off the key.

orblivion commented

Another issue I found (and now fixed) while fixing the vagrant-spk issues was that the app did not work on the latest version of Firefox. This was Sandstorm-specific.

My fix is a selective revert of a commit that I don't really understand. So, this would be a good one to review if anybody understands browser quirks particularly well.

Error symptom: I get a popup that tells me this error message:

Error: Ace2Editor.init() error event while waiting for load event
at http://ui-25116f71e976f7bb6facfbdf8604de2b.local.sandstorm.io:6080/javascripts/lib/ep_etherpad-lite/static/js/pad.js?callback=require.define&v=45902aac at line 2994
ErrorId: BH7HwXFZrg2tFoykmHlb
Unhandled Promise rejection
URL: http://ui-25116f71e976f7bb6facfbdf8604de2b.local.sandstorm.io:6080/p/main
UserAgent: Mozilla/5.0 (X11; Linux x86_64; rv:96.0) Gecko/20100101 Firefox/96.0

This seems related: ether/etherpad-lite#4975 However it seems to apply to Firefox overall, whereas I'm only seeing this in Sandstorm. Or maybe I'm misreading this.

I bisected and found that it was introduced by this commit, a fix for Safari:

ether/etherpad-lite@cb9f6d6

Here is my fix (undo the above only for Firefox):

sandstorm-org/etherpad-lite-sandstorm@2e1efa0#diff-ddc6a036badf142ae4178f202df2272d02014f8fe2853064f09972ea8ef43027R1

orblivion commented

I am absolutely good with it if it only works in spk, though if it works in both, it'd be a relatively nice perk.

If nothing else I could get more eyes on it now if I get stuck on something.

I think really the only thing missing for publishing is making sure the upgrade works from the old Etherpad, and then seeing if we can get Kenton to review it, and decide how to hand off the key.

I've actually already made a sqlite3 file with the previous sandstorm app and put it into a testing directory for future regression testing. I even made a dirty.db file from the very first version (that you helped me find) for the same reason. Turns out that one migrates mostly fine as well. And, I've already written code for backing up the sqlite3 file when we switch to this version of the app (I change the db file name so I know when we switch versions, as you suggested).

However, right now the paths within the app are totally different from Kenton's. Part of it is understandable because the app is not a fork of etherpad anymore, but part of it may be quirks in the files list system. I need to get back to figuring that out now.

Beyond that, though, I've collected a long list of small issues that I've passed on along the way in my rush to hit the bullet points. I'm probably gonna post them soon so we can evaluate. Given track record I imagine a lot of these you guys will dismiss as nbd but I want it on the record, for future improvements if nothing else.

zenhack commented

Hm, I am still seeing that error in firefox when I pull your latest branch and do vagrant-spk dev. Also I am not seeing that error on alpha with the previous spk, even in firefox. It does work in chromium.

Re: vagrant-spk pack: I think it would be worth making a token effort to get it working, but I am fine punting if it looks like it's going to be another rabbit hole, as long as we're able to build it somehow.

Getting close, I'm excited.

orblivion commented

The aforementioned long list of small issues that are worth evaluation:

https://github.com/orblivion/etherpad-lite-sandstorm/blob/main/ISSUES.md

ocdtrekkie commented

As a Firefox user, that first one really hits hard. :P

I am curious if upstream might be willing to provide some drive-by insight on a couple of the "is this Etherpad, or is this Sandstorm specific" questions. Asking upstream for significant effort would be unfair for our... fairly unique implementation of the software, but I am wondering if we might be able to get some quick insight on some of these items. They also might just be interested to know what we're working on.

It looks like John McLear may have handed off Etherpad maintainership for the time being to @rhansen?

orblivion commented
ocdtrekkie commented

Oh cool. I need to pay more attention to Etherpad happenings, but I have a newborn.

Do these issues apply to the package submitted to experimental? Because I am able to use that on Firefox 96.0.2, and I pasted the entirety of the ISSUES.md contents into a pad.

zenhack commented

I've had the problem on firefox 96, but per above but only on my local dev install; it works fine on alpha. Note that I have not tried this on my own production box just because I'm a little nervous about trashing etherpad grains I'm actively using, though in principle it shouldn't break anything.

orblivion commented
ocdtrekkie commented

Alpha is just the server I chose to install your experimental SPK on for testing. It doesn't select SPKs any differently than any other Sandstorm server.

zenhack commented

As mentioned here I did also try it with vagrant-spk dev and saw the same problem on my local dev install.

So, to be clear, with firefox 96:

  • On my local dev install:
    • The SPK in experimental gives me that error.
    • Running vagrant-spk dev with the latest code gives me the same error.
  • On Alpha:
    • The SPK in experimental works fine
    • I have not tried the latest on the repo, since I don't have an spk to run.
rhansen commented

Is the Firefox bug you are talking about https://bugzilla.mozilla.org/show_bug.cgi?id=1721296 ? If so, that was worked around in ether/etherpad-lite#5124 (Etherpad 1.8.15 and later).

zenhack commented

That looks promising. @orblivion, maybe worth updating the etherpad version. (I took a stab at this, but a few of the patches have conflicts and I didn't want to take the time right now to fix them).

orblivion commented
zenhack commented
orblivion commented

Since we're putting off the caching/minimizing thing for now (and again I'm not sure how much that would even buy us) I'm going to turn off caching altogether so as to not pollute the grain. I could also write it to /tmp/ so that it sticks around until the grain quits. But I'm not sure it makes much if any noticeable difference.

Some background in case this is confusing:

The way Etherpad works out of the box is that it minimizes, maybe gzips, and puts the result in the etherpad-lite/var/ directory (which we've mapped to within the grain, since the db also goes here). It has to do this on the first request of every grain.

The optimal way (the one Kenton ultimately did, and the one we're putting off) is to run this process during packaging time, use the cache/ directory instead of the var/ directory, where cache/ is within the package. Now it'll use the cached responses from the very first run of any grain.

Since we're putting that off, I turned off minimizing a while ago because it's really slow. But it still pollutes the grain by putting un-minimized cached files into the var/ directory. So now (what I'm letting people know about here) I'm just turning off caching.

Before Kenton implemented the optimal minimization cache, he had it to write to a temp directory instead of var/ for this reason. (Except, it looks like he was accidentally writing to etherpad-lite/tmp/ and I don't think that exists. The way it's implemented, if the caching directory doesn't exist, it just skips caching. So, I think Kenton accidentally just turned off caching at this point. Of course he fixed it when he implemented the optimal strategy.)

At any rate, I don't notice any change in speed whether it's /tmp/, var/, or not cached at all.

zenhack commented

Any updates?

orblivion commented

I made a change and did some experimenting that cleared up a lot of confusion (in my mind, anyway) about sandstorm-files.list, alwaysInclude, hidePaths, etc. It was a big ambiguous item in my mind, so it's good that it's out of the way. (There is one weird thing left: inconsistency as to whether the files under the bin/ lib/ lib64/ symlink dirs end up automatically populated in sandstorm-files.list. But this is more about preventing confusion when developing future versions. I guess I could just add it to ISSUES.md.)

I made another change related to file paths inside the grain. @zenhack had the Etherpad var directory pointing to /var/etherpad-lite-var, I'm pointing it straight to /var as Kenton effectively did. With this change, I expect that old grains should work. This also took a lot of time to consider how I was going to handle it, even though the change is fairly small. So that's another big item off my mind.

orblivion commented

What's left is the Firefox item (since that was the one item from ISSUES.md that I was told was worth fixing), and then some packaging issues basically. And maybe one other problem that I made a note to myself about, but I'm not sure why.

And I think that's it.

zenhack commented

I think I have a lead on the firefox issue: it appears to only occur when you have ALLOW_LEGACY_RELAXED_CSP=false set in sandstorm.conf.

Looking at the console, there are numerous csp errors, but they seem to be referring to urls that are hosted internally, but that are being pulled in as a result of javascript injecting stuff into the page (e.g. adding an element that pulls in a stylesheet at runtime). So this might actually be a bug in our CSP handling, and also might not be a blocker after all. I'll look into it more and report back.

zenhack commented

Ok, I stared at our CSP implementation for a bit, and I strongly suspect that this is related to that firefox bug (esp. since it works fine in chromium). I'm comfortable punting on this for the etherpad release, as it doesn't affect most prod setups and is probably either a sandstorm issue or a firefox issue.

So I think what's left:

  • appMarketingVersion is still set to the string "TODO".
    • Probably should do a quick scan over to make sure all the metadata is filled out.
  • Figure out who's going to do the official build, and maybe change the appId.
    • If we use the old app's appId, we should confirm upgrading old grains works.

I think it's probably best to use the old appId. In which case, @kentonv needs to either do the build himself or hand off the key to someone.

orblivion commented

it doesn't affect most prod setups

Well, we can put the app out as experimental and see if it breaks for Firefox users, yeah? Or do you mean we'll let it break for Firefox users in hopes that it'll be fixed soon?

Other than that, yeah I just have to do the packaging stuff as you described and I think that's it.

I already had in my notes to remember to set appMarketingVersion and to check for other things to set in pkdgef. (I'm assuming we should leave pgp-keyring and pgp-signature to Kenton).

Also, README and CHANGELOG. (I want to factor in Kenton's old CHANGELOG).

Though, I added a couple items to ISSUES.md, which I'm guessing you'll want to punt on if address at all:

The checkboxes and hours in the issue description need to be updated. It got a little complicated since there was wrapup stuff not really in any category.

zenhack commented
orblivion commented

Kenton seems to usually set appMarketingVersion to <etherpad-version>~<release-date>. Maybe I'll put the etherpad version and TODO for him to fill in when the time comes?

zenhack commented

Maybe @kentonv can comment on why he used that convention rather than the upstream version number?

orblivion commented

Okay, I looked through the pkgdef and compared it to Kenton's. A few questions remain:

  • CHANGELOG - as described before. that's a TODO for me. (actually maybe significant since a lot may have changed).
  • The first line in the pkgdef file that begins with @ - what's up with that? Does it need to be the same as Kenton's in order for it to be considered the same app?
  • appMarketingVersion - as we described
  • codeURL - should it stay pointing to my github or do we want to host the canonical version on someone else's account?
  • contactEmail - related - should this be me?
  • pgp-keyring - Kenton will do
  • pgp-signature - Kenton will do
  • hidePaths - Related to this, you previously said:

once the files are removed from sandstorm-files.list, they shouldn't be re-added unless spk dev is run in an environment where they're in the old place.

Are you saying that paths such as /bin/bash were only in sandstorm-files.list in the first place because you (@zenhack) initially created the repo on your system? If not, then they must have been added somehow on my system, where /bin is a symlink. In which case, for all I know they could reappear some day (even though they're not reappearing now, which is the mystery I laid out in ISSUES.md)

But if I understood your explanation right, then I'll delete that entry in ISSUES.md as well as the file paths in hidePaths.

orblivion commented

Yeah, per your initial commit, that seems to be the case. /bin/bash is there but not /usr/bin/bash. I guess that explains it.

ocdtrekkie commented

Re: What should be Kenton's versus yours, it should be who is responsible the release to some degree. I am unsure if @kentonv would prefer to fork and release this, or if he'd rather hand over or rotate the app key to you. (And I suppose your interest in continuing to do that may also be a relevant question there.)

It's even possible/worth asking if someday we should maintain core app packages within the sandstorm-io org at some future point.

zenhack commented
orblivion commented

Fork the repo to the sandstormports org and put sandstorm-dev@googlegroups.com

Okay cool. I went with this. I guess for simplicity I'll remind you to fork after I'm done with my few remaining changes. Unless you just want to give me write access to it or do pull requests.

orblivion commented

Okay I got one small question that comes to mind: I am generating and writing SESSION.txt inside /tmp. As I understand, this means that on each server restart, the existing sessions should break. That could be a problem. However, isn't it the case that Sandstorm refreshes everybody's page on server restart regardless? If not, I'll think of something else for SESSION.txt. No problem saving to grain (I did that previously) I just figured there's no use polluting the filesystem if I can avoid it (it's not a space concern, it's tiny).

If I'm not changing that, I think I'm done making commits until Kenton is ready for the three items I need from him. You can fork the repo to sandstormports.

ocdtrekkie commented

So, I am thinking I should ask @xet7 to maybe make me an owner of the org here. (Apparently I am a "member" of sandstormports), so that I can in turn add other people to the org. Would it make sense to fork the repo here or figure out to make it so you can transfer it here, so issues and everything remains intact, and if you need to update it, you can maintain access to the canonical repo?

orblivion commented

Yeah totally. I'll try to do that now.

orblivion commented

The transfer of ownership thing.

orblivion commented

Oh you probably can't accept the transfer without being an owner, nevermind. I'll wait.

orblivion commented

Okay good deal. I joined the organization and transferred ownership. I can still push to it.

orblivion commented

Regarding the session thing (literally the only thing left on my list) - I tried sharing a document, and then restarting the app with the restart button.

It reloads the page on the owner's side, but not on the side of the shared app. I see from the logs on the owner's side that it is generating a new SESSION.txt. However, after the app reloads, the person shared with is still able to communicate edits in both directions. I did not expect this.

I guess it means it works at least for this version. I don't know how this would happen other than that the etherpad session key isn't used at all when running in sandstorm. Or maybe it reestablishes the session without reloading the page.

I'd only worry about what happens in future version if the session key somehow becomes relevant and there are hidden connection issues for shared collaborators when the app restarts.

zenhack commented

I don't have a strong opinion re: what to do about the session thing; you could move it to /var just in case, but if it appears to be working I'm comfortable punting until and unless a problem comes up testing a future version.

orblivion commented

Okay I just moved it to /var and we won't have to think about it. Also a few more files ended up in sandstorm-files.list.

That's it, afaik the next step is for Kenton to examine it, sign it, etc. BTW he'll hit an error for lack of pgp files referred to in pkgdef. He can delete those lines in pkgdef until he's ready to sign.

ocdtrekkie commented

Going to close this one, experimental app seems good enough to consider this done.