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.

Merged
merged 32 commits into from Nov 26, 2019

Conversation

poljar
Copy link
Contributor

@poljar 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
Copy link
Member

t3chguy commented Oct 11, 2019

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

Copy link
Contributor

@jryans 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"
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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",
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Contributor Author

@poljar poljar Nov 8, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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);
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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) => {
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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');
Copy link
Contributor

@jryans jryans Nov 6, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
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
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 poljar requested a review from jryans Nov 19, 2019
Copy link
Contributor

@jryans 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
Contributor

@jryans 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.
Copy link
Contributor

@jryans jryans Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jryans jryans Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Contributor Author

@poljar poljar Nov 25, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@jryans jryans Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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"
Copy link
Contributor

@jryans jryans Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.
Copy link
Contributor

@jryans jryans Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Contributor

@jryans 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.

docs/labs.md Outdated
@@ -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`)
Copy link
Contributor

@jryans jryans Nov 26, 2019

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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
Web App Team automation moved this from In Review to In Test Nov 26, 2019
@jryans jryans added the phase:1 label Feb 10, 2020
@poljar poljar deleted the poljar/seshat-pr branch Jun 21, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants