Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

ElectronPlatform: Add support for a event index using Seshat. #11125

Merged
merged 32 commits into from Nov 26, 2019
Merged

Conversation

@poljar
Copy link
Contributor

poljar commented Oct 11, 2019

This is the platform specific part of our E2EE search support. The ElectronPlatform is extended to make use of Seshat.

@t3chguy

This comment has been minimized.

Copy link
Collaborator

t3chguy commented Oct 11, 2019

The flow type annotations in ElectronPlatform are a bit un-useful with {}

@jryans jryans requested a review from vector-im/riot-web Oct 28, 2019
@jryans jryans added this to In Progress in Workflow Oct 28, 2019
@t3chguy t3chguy moved this from In Progress to In Review in Workflow Oct 28, 2019
@turt2live turt2live requested review from bwindels and jryans and removed request for vector-im/riot-web Nov 4, 2019
@jryans jryans removed the request for review from bwindels Nov 6, 2019
Copy link
Member

jryans left a comment

Thanks, looks like a good starting point!

If you'd like to work towards merging this before we have a build process ready for native modules, then we should do something like:

  • Test for the native node module(s) needed with runtime checks
  • Do not add the native node module as an app dependency (instead record what should be added in a doc)

The push-to-talk PR (which is in a similar boat) has a draft of docs around native node modules, so perhaps we can describe the manual steps needed in something like that.

As for the seshat backend itself, as you've seen, I filed various issues over there, but most are smaller nits that can be addressed whenever. I would like to see the following resolved in some way or another before merging integration into Riot:

@@ -5,11 +5,17 @@
"version": "1.4.2",
"description": "A feature-rich client for Matrix.org",
"author": "New Vector Ltd.",
"scripts": {
"build": "electron-build-env --electron 6.0.3 neon build seshat-node --release",
"postinstall": "yarn build"

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

Hmm, so this is just to call the line above...?

@@ -5,11 +5,17 @@
"version": "1.4.2",
"description": "A feature-rich client for Matrix.org",
"author": "New Vector Ltd.",
"scripts": {
"build": "electron-build-env --electron 6.0.3 neon build seshat-node --release",

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

We should really try hard to find a way to build that doesn't duplicate the Electron version in another place, as we'll surely forget to keep them synchronised. Maybe we need a wrapper script to pull it out of the main package.json?

This comment has been minimized.

Copy link
@poljar

poljar Nov 8, 2019

Author Contributor

The thing doesn't even work with the version passed as is. It seems to be only necessary if you need to rebuild seshat for some reason (e.g. if you're linking it from your dev folder) anyways.

Since we're adding run-time checks for seshat anyways it will be safe to remove it as well.

"png-to-ico": "^1.0.2"
"png-to-ico": "^1.0.2",
"make-dir": "^3.0.0",
"seshat-node": "^0.2.0"

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

It would be quite nice to have this renamed to something without the redundant node instead. See matrix-org/seshat#9.

let p = path.normalize(path.join(eventStorePath, args[0]));
try {
await makeDir(p);
eventIndex = new seshat(p);

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

A common JS convention is for new-able things to start with a capital letter. I suppose we may not have a linting rule enforcing that at the moment, but I suggest changing the require to call it Seshat instead, unless there's a strong reason to always use the lowercase name.

@@ -149,6 +155,17 @@ autoUpdater.on('update-downloaded', (ev, releaseNotes, releaseName, releaseDate,
ipcMain.on('ipcCall', async function(ev, payload) {
if (!mainWindow) return;

const send_error = (id, e) => {

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

Use camel-case naming:

Suggested change
const send_error = (id, e) => {
const sendError = (id, e) => {

(We should have linting rules for this, but they seem not to have fired here... 🤔)

@@ -39,6 +39,8 @@ const { migrateFromOldOrigin } = require('./originMigrator');

const windowStateKeeper = require('electron-window-state');
const Store = require('electron-store');
const seshat = require('seshat-node');

This comment has been minimized.

Copy link
@jryans

jryans Nov 6, 2019

Member

As part of attempting to land this without a path to build it, let's test at runtime for its existence with a try / catch.

@poljar

This comment has been minimized.

Copy link
Contributor Author

poljar commented Nov 13, 2019

This should be ready for another look. The thing that's missing is switching from seshat-node to matrix-seshat, enabling encryption, and removing the stuff from package.json. Will be a trivial fix, once I get matrix-seshat published.

@poljar poljar requested a review from jryans Nov 13, 2019
@poljar

This comment has been minimized.

Copy link
Contributor Author

poljar commented Nov 14, 2019

The logout logic, and therefore event index deletion required a bit more changes here since I wrongly scoped our event index to a user id.

Should now be ready.

@poljar

This comment has been minimized.

Copy link
Contributor Author

poljar commented Nov 19, 2019

Updated everything except the error propagation for event index deletions and I still didn't remove the deps from the package.json. Rest should be fine.

The merge conflict is again simple and caused by labs flags.

@poljar poljar requested a review from jryans Nov 19, 2019
Copy link
Member

jryans left a comment

Thanks for the continued work here. It looks reasonable enough, but I'd like to review the final form with dep removed, documentation to enable, etc. so I can check the approach, see how it aligns with the push-to-talk PR, etc.

If you'd like another review Thursday - Friday while I am away, feel free to request review from the riot-web team, or else we can wait until I am back next week.

@poljar poljar requested a review from jryans Nov 25, 2019
Copy link
Member

jryans left a comment

I realise you copied some of this text from the Push-to-talk PR. I hadn't reviewed it in depth over there yet, as it was still unclear how it would all come together...

Since it looks like you'll be the first to merge with native modules, that means the detailed text review is happening here. Apologies for all my copy edits, but I want make sure we convey the right message in these docs.

docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
docs/native-node-modules.md Outdated Show resolved Hide resolved
If for some reason recompilation of Seshat is needed, e.g. when using a
development version of Seshat using `yarn link`, or if the initial compilation was
done for the wrong electron version, Seshat can be recompiled with the
`electron-build-env` tool. Again from the `electron_app/` directory:

yarn add electron-build-env

Recompiling Seshat itself can be done like so:

yarn run electron-build-env -- --electron 6.1.1 -- neon build matrix-seshat --release`

Please make sure to include all the `--` as well as the `--release` command line
switch at the end. Modify your electron version accordingly depending on the
version that is installed on your system.
Comment on lines +31 to +44

This comment has been minimized.

Copy link
@jryans

jryans Nov 25, 2019

Member

Hmm, so is electron-build-env needed even for the first installation of the matrix-seshat Node module, or is only a rebuild helper?

If it's only used for rebuilds, I wonder if we can find some way to repeat the build that happens at install time without this extra tool somehow, such as (cd node_modules/matrix-seshat; yarn run install) or similar.

This comment has been minimized.

Copy link
@jryans

jryans Nov 25, 2019

Member

I also noticed that the Electron docs suggest electron-rebuild, is that of any use here?

This comment has been minimized.

Copy link
@poljar

poljar Nov 25, 2019

Author Contributor

I believe it's only needed to rebuild it, but I'm not 100% on that.

The neon docs state this:

We are working on adding Neon support to electron-rebuild, so you'll be able to just drop Neon dependencies into your app like any other. For now, there's a tool called electron-build-env that you can use for building any Neon dependencies of your Electron app.

As far as I can tell this has not changed as of yet.

This comment has been minimized.

Copy link
@jryans

jryans Nov 26, 2019

Member

After looking around a bit, it's quite difficult to follow what's actually recommended and also matches what we need here. 😓 I don't want to block all this work on sorting it out though, so I filed a separate issue to improve this in a future step.

package.json Outdated Show resolved Hide resolved
@@ -27,7 +27,8 @@
"feature_sas": "labs",
"feature_room_breadcrumbs": "labs",
"feature_state_counters": "labs",
"feature_many_integration_managers": "labs"
"feature_many_integration_managers": "labs",
"feature_event_indexing": "labs"

This comment has been minimized.

Copy link
@jryans

jryans Nov 26, 2019

Member

Please also add a short explanation of this flag in the labs docs as per the guide.

docs/native-node-modules.md Show resolved Hide resolved
If for some reason recompilation of Seshat is needed, e.g. when using a
development version of Seshat using `yarn link`, or if the initial compilation was
done for the wrong electron version, Seshat can be recompiled with the
`electron-build-env` tool. Again from the `electron_app/` directory:

yarn add electron-build-env

Recompiling Seshat itself can be done like so:

yarn run electron-build-env -- --electron 6.1.1 -- neon build matrix-seshat --release`

Please make sure to include all the `--` as well as the `--release` command line
switch at the end. Modify your electron version accordingly depending on the
version that is installed on your system.

This comment has been minimized.

Copy link
@jryans

jryans Nov 26, 2019

Member

After looking around a bit, it's quite difficult to follow what's actually recommended and also matches what we need here. 😓 I don't want to block all this work on sorting it out though, so I filed a separate issue to improve this in a future step.

@poljar poljar requested a review from jryans Nov 26, 2019
@jryans
jryans approved these changes Nov 26, 2019
Copy link
Member

jryans left a comment

As always, thanks for working through all the nits and copy edits here! 😁

Looks like a few conflicts to resolve, but should be ready to merge. Assuming GH will let you, it's fine to merge without another review once those are resolved.

@@ -49,3 +49,11 @@ That's it. Now should see your new counter under the header.
## Multiple integration managers (`feature_many_integration_managers`)

Exposes a way to access all the integration managers known to Riot. This is an implementation of [MSC1957](https://github.com/matrix-org/matrix-doc/pull/1957).

## Event indexing or E2EE search support using Seshat (`feature_event_indexing`)

This comment has been minimized.

Copy link
@jryans

jryans Nov 26, 2019

Member

Probably "and" is more natural since Seshat supports both.

Suggested change
## Event indexing or E2EE search support using Seshat (`feature_event_indexing`)
## Event indexing and E2EE search support using Seshat (`feature_event_indexing`)
@poljar poljar merged commit e5956de into develop Nov 26, 2019
5 checks passed
5 checks passed
buildkite/riot-web Build #1406 passed (3 minutes, 20 seconds)
Details
buildkite/riot-web/eslint-lint Passed (41 seconds)
Details
buildkite/riot-web/i18n Passed (2 minutes)
Details
buildkite/riot-web/karma-tests Passed (3 minutes, 6 seconds)
Details
buildkite/riot-web/pipeline Passed (4 seconds)
Details
Workflow automation moved this from In Review to In Test Nov 26, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.