Dan: Upgrade Etherpad package (to 1.18.14 or close) #15
Comments
|
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. |
|
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. |
|
All sounds good to me. I am merely curious if someone could post a few sentences explaining the vagrant-spk -> spk change (motivation, etc.). |
|
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. |
|
Also, I think we should send out a thing to the mailing list and wait a day or two before pulling the trigger. |
|
(I sent out a thing) |
|
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). |
|
Yeah, if there's a way it's not very obvious. It doesn't work out of the box at any rate. |
|
Alright, it's been few days and no one has spoken up; I think we can give you the green light. |
|
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). |
|
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. |
|
Great!
Yeah, this sounds surmountable to me. |
|
Next is installing dependencies. See latest on my repo for the results of that. (4 hours so far.) I initially included the altered plugins ( 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). Resultssqlite3Ian already had taken care of sqlite3. capnpI had to wrestle a bit with capnp but I got it to the point where I'm able to unaltered pluginsThey 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_attributesThis 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 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_markdownExisting 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. |
|
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. |
|
I reported the two New Bugs right after posting this. Maybe I could fix
them, actually.
…On Tue, Oct 5, 2021 at 3:10 PM Jacob Weisz ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKH6HFVWXIUPQZ4EDZCBLUFNEQVANCNFSM5E6W6X3Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
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. |
|
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. |
|
The only reason I care about milestone bookkeeping is to compare estimated
and actual times. You have the option to pull the plug on this whole thing
based on that, so I want to give you useful data.
…On Tue, Oct 5, 2021 at 5:05 PM Ian Denhardt ***@***.***> wrote:
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.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#15 (comment)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAAKH6BVCYPRFSV3WBY3N43UFNSCJANCNFSM5E6W6X3Q>
.
Triage notifications on the go with GitHub Mobile for iOS
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675>
or Android
<https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub>.
|
|
Fair enough. In any case, what you suggested is fine with me.
Quoting Daniel Krol (2021-10-05 18:03:52)
… The only reason I care about milestone bookkeeping is to compare
estimated
and actual times. You have the option to pull the plug on this whole
thing
based on that, so I want to give you useful data.
On Tue, Oct 5, 2021 at 5:05 PM Ian Denhardt ***@***.***>
wrote:
> 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.
>
> �
> You are receiving this because you were mentioned.
> Reply to this email directly, view it on GitHub
>
<#15 (comment)
ment-934834104>,
> or unsubscribe
>
<https://github.com/notifications/unsubscribe-auth/AAAKH6BVCYPRFSV3WBY3
N43UFNSCJANCNFSM5E6W6X3Q>
> .
> Triage notifications on the go with GitHub Mobile for iOS
>
<https://apps.apple.com/app/apple-store/id1477376905?ct=notification-em
ail&mt=8&pt=524675>
> or Android
>
<https://play.google.com/store/apps/details?id=com.github.android&refer
rer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source
%3Dgithub>.
>
>
--
You are receiving this because you were mentioned.
Reply to this email directly, [1]view it on GitHub, or [2]unsubscribe.
Triage notifications on the go with GitHub Mobile for [3]iOS or
[4]Android.
Verweise
1. #15 (comment)
2. https://github.com/notifications/unsubscribe-auth/AAGXYPV5CNMYG2JTFTK5PY3UFNY4RANCNFSM5E6W6X3Q
3. https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
4. https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
|
|
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? |
|
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. |
|
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. |
|
Okay. I'll call this milestone done and move on for now then. |
|
Agreed; I'm not inclined to spend time fussing with minor upstream bugs & regressions, unless they somehow disproportionately affect Sandstorm users. |
|
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. |
|
@zenhack I found that you have |
|
Quoting Daniel Krol (2021-10-20 12:48:50)
***@***.*** 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?
I don't think the difference is critical; I kindof like rootfs for
config like that better conceptually, but either way will certainly
work.
|
|
Quoting Daniel Krol (2021-10-20 12:15:39)
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.
Thanks for keeping us posted. If something was going to go over time I'm
not surprised it was this; it looked pretty hairy when I peeked at it. I
think it's important though, since it has big implications for
performance.
…-Ian
|
|
I *think* I actually tried this on the most recent Etherpad with no
difference. And I think this is actually Sandstorm specific. Which kind of
makes sense since it has to do with frames.
I'll test these things again and report my specific results. In retrospect
I should have done this the first time.
|
|
Either way, probably worth being on the latest release.
Quoting Daniel Krol (2022-01-27 12:46:59)
… I *think* I actually tried this on the most recent Etherpad with no
difference. And I think this is actually Sandstorm specific. Which kind
of
makes sense since it has to do with frames.
I'll test these things again and report my specific results. In
retrospect
I should have done this the first time.
--
Reply to this email directly, [1]view it on GitHub, or [2]unsubscribe.
Triage notifications on the go with GitHub Mobile for [3]iOS or
[4]Android.
You are receiving this because you were mentioned. Message ID:
***@***.***>
Verweise
1. #15 (comment)
2. https://github.com/notifications/unsubscribe-auth/AAGXYPVDQ3YTET3V7JFXMTLUYGAJHANCNFSM5E6W6X3Q
3. https://apps.apple.com/app/apple-store/id1477376905?ct=notification-email&mt=8&pt=524675
4. https://play.google.com/store/apps/details?id=com.github.android&referrer=utm_campaign%3Dnotification-email%26utm_medium%3Demail%26utm_source%3Dgithub
|
|
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 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 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 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 Before Kenton implemented the optimal minimization cache, he had it to write to a temp directory instead of At any rate, I don't notice any change in speed whether it's |
|
Any updates? |
|
I made a change and did some experimenting that cleared up a lot of confusion (in my mind, anyway) about I made another change related to file paths inside the grain. @zenhack had the Etherpad var directory pointing to |
|
What's left is the Firefox item (since that was the one item from And I think that's it. |
|
I think I have a lead on the firefox issue: it appears to only occur when you have 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. |
|
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:
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. |
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 Also, README and CHANGELOG. (I want to factor in Kenton's old CHANGELOG). Though, I added a couple items to
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. |
|
Quoting Daniel Krol (2022-03-18 18:44:55)
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?
Once it's in experimental we should probably ping the mailing list for a
bit of beta testing in general, but as long as it's only broken with
that option set I don't think it should block.
Other than that, yeah I just have to do the packaging stuff as you
described and I think that's it.
Great! Keep us posted.
Though, I added a couple items to ISSUES.md, which I'm guessing you'll
want to punt on if address at all:
* [1]https://github.com/orblivion/etherpad-lite-sandstorm/blob/main/I
SSUES.md#do-i-actually-need-all-the-hidefiles-stuff-under-the-symli
nk-directories
I don't think there's anything that needs doing here; 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. The
only potentially actionable thing is the fix I suggested to spk itself,
but that obviously doesn't concern the etherpad package specifically.
* [2]https://github.com/orblivion/etherpad-lite-sandstorm/blob/main/I
SSUES.md#spk-size---usrlibnode_modules
Yeah, I don't think this is worth worrying about.
…-Ian
|
|
Kenton seems to usually set |
|
Maybe @kentonv can comment on why he used that convention rather than the upstream version number? |
|
Okay, I looked through the pkgdef and compared it to Kenton's. A few questions remain:
Are you saying that paths such as But if I understood your explanation right, then I'll delete that entry in |
|
Yeah, per your initial commit, that seems to be the case. |
|
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. |
|
Quoting Daniel Krol (2022-03-19 18:57:34)
* CHANGELOG - as described before. that's a TODO for me. (actually
maybe significant since a lot may have changed).
You could probably just say "updated to much newer version" and maybe
link to this issue; @ocdtrekkie will be the one reviewing it, and it
only shows up in the submission, not on the app market, so it's not like
it needs to be understandable by someone without context anyway.
* 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?
This is a capnproto thing. Doesn't matter for app packaging purposes.
* 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?
I see options:
1. Fork the repo to the sandstormports org and put sandstorm-dev@googlegroups.com
2. Have @kentonv take over
3. Put yourself here
I would only do (3) if you want to commit to being the long-term
maintainer of the app, which we haven't asked you to do.
My guess is that putting Kenton's email on the app probably doesn't
make a ton of sense, since I imagine he isn't going to find much if any
time to actually do maintenece -- so if someone sends mail about the
app we probably want other people to see it.
* 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 ***@***.***)
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.
+1
|
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. |
|
Okay I got one small question that comes to mind: I am generating and writing 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. |
|
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? |
|
Yeah totally. I'll try to do that now. |
|
The transfer of ownership thing. |
|
Oh you probably can't accept the transfer without being an owner, nevermind. I'll wait. |
|
Okay good deal. I joined the organization and transferred ownership. I can still push to it. |
|
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 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. |
|
I don't have a strong opinion re: what to do about the session thing; you could move it to |
|
Okay I just moved it to /var and we won't have to think about it. Also a few more files ended up in 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. |
|
This is now in the experimental app market: https://apps.sandstorm.io/app/h37dm17aa89yrd8zuqpdn36p6zntumtv08fjpu8a8zrte7q1cn60?experimental=true |
|
Going to close this one, experimental app seems good enough to consider this done. |
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.)
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
Caveat on vagrant-spk -> spk:
spk packed version can't find any files in the package./usr/bin/bash, notlauncher.sh, nothing.Based on Kenton's changes:
Sandstorm Packaging Stuff
Trivial items
Style
ep_author_neat)Minifying/Caching/Performance
Dependencies
Install plugins, including two modified plugins ((these have their own milestones)ep_comments_pageandep_author_neat) from source.Migrating dirty.db -> sqlite
Sandstorm Profile Integrations
ep_author_neat(see below).Sandstorm Auth/Permission Integrations
Sandstorm Activities Integrations
Assorted
ep_author_neatep_comments_pageThe text was updated successfully, but these errors were encountered: