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

chore(deps): Updated to Theia 1.39.0 #2144

Merged
merged 20 commits into from
Aug 14, 2023
Merged

chore(deps): Updated to Theia 1.39.0 #2144

merged 20 commits into from
Aug 14, 2023

Conversation

kittaakos
Copy link
Contributor

@kittaakos kittaakos commented Jul 18, 2023

Motivation

  • Update the Theia dependencies to 1.39.0.
  • Bundled the backend application with webpack:
    • To reduce the final application size (from 552MB to 494MB on macOS), and
    • Speed up backend/electron main processes startup due to the fewer Node.js require calls.
  • Refactored the application packager code:
    • Removed the private npm registry and custom build script, and
    • Fixed the security vulnerabilities.
      % yarn audit
      yarn audit v1.22.18                                                                                                                                                  
      0 vulnerabilities found - Packages audited: 1984                                                                                                                     
      ✨  Done in 2.14s.               

Other noteworthy changes:

  • [ci]: Enabled yarn caching in the CI.
  • [dev]: No magic npm scripts. From now on, executing yarn won't build the code, but there is a series of scripts to execute to build, test, and bundle the application.
  • [dev]: Concise task and launch configuration naming.
  • [dev]: Moved all the binaries and examples from arduino-ide-extension/build to arduino-ide-extension/src/node/resources.
  • [dev]: IDE2 stores the version of the required binaries in the arduino-ide-extension/package.json under the arduino property. From now on, the executable keys match the executable filename.
  • [dev]: Turned the asynchronous @postConstruct methods to synchronous. The latest InversifyJS version requires it (inversify/InversifyJS#1482).
  • [dev]: Updated to latest lerna.
  • [dev]: From now on, when the built-in examples are already cloned, the build script won't clone it. Previously, the local repository clone was erased.

Change description

Other information

As the PR author, I expect no functional changes in IDE2 after merging this PR.

TODO for @kittaakos:

Reviewer checklist

  • PR addresses a single concern.
  • The PR has no duplicates (please search among the Pull Requests before creating one)
  • PR title and description are properly filled.
  • Docs have been added / updated (for bug fixes / features)

@kittaakos kittaakos added type: enhancement Proposed improvement topic: infrastructure Related to project infrastructure topic: code Related to content of the project itself topic: theia Related to the Theia IDE framework labels Jul 18, 2023
@kittaakos kittaakos self-assigned this Jul 18, 2023
electron-app/scripts/post-package.js Outdated Show resolved Hide resolved
electron-app/scripts/post-package.js Outdated Show resolved Hide resolved
@kittaakos kittaakos force-pushed the app-packager branch 3 times, most recently from 7be3cec to 2b37c98 Compare July 19, 2023 19:33
@kittaakos kittaakos added the topic: security Related to the protection of user data label Jul 19, 2023
@kittaakos kittaakos marked this pull request as ready for review July 20, 2023 08:06
.vscode/launch.json Outdated Show resolved Hide resolved
electron-app/package.json Show resolved Hide resolved
@kittaakos
Copy link
Contributor Author

I have updated the VSIX language packs to VS Code 1.78.0. (See why 1.78.0.)

Some screenshots from the IDE2 using Italian as the language. (See some language issues with the 1.37.0 Theia update here.)

Screenshot 2023-07-25 at 14 31 32 Screenshot 2023-07-25 at 14 31 28 Screenshot 2023-07-25 at 14 31 22 Screenshot 2023-07-25 at 14 31 08 Screenshot 2023-07-25 at 14 31 01

Please review. Thank you!

@kittaakos
Copy link
Contributor Author

kittaakos commented Jul 26, 2023

Must be aligned with #2151.

✅ Done

 - [security]: removed the app packager
 - [feat]: bundle the backend app with webpack

Signed-off-by: Akos Kitta <[email protected]>
Copy link
Contributor

@AlbyIanna AlbyIanna left a comment

Choose a reason for hiding this comment

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

I tested as many feature as I could on MacOs (Ventura 13.4) and couldn't spot any issue! LGTM ✅

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: Fixed by 3529b14

Incorrect layout of editor tab dialog error message highlight

Selecting "New Tab" or "Rename" (if a secondary sketch file tab is selected) from the editor toolbar context menu causes a dialog to open. The user input in the tab name field is validated. If the validation fails, an error message is printed in the dialog.

🐛 The position of the colored background on this text is off-center and doesn't have any padding for the dialog's buttons:

image

To reproduce

  1. Select "New Tab" from the editor toolbar context menu.
  2. In the "Name for new file" field, type foo bar

The expected error message appears:

The name must start with a letter, number, or underscore, followed by letters, numbers, dashes, dots and underscores. Maximum length is 63 characters.

🐛 but the position of the colored background on this text is off-center and doesn't have any padding for the dialog's buttons.

Expected behavior

The error message highlight is centered on the text and has padding against the buttons.

-OR-

The error message is not highlighted.

Arduino IDE version

090098d (tester build for e8a9ef7)

Operating system

Windows 11

Additional context

The highlighting behavior is something introduced by the PR. There is no such highlight on the error message when using the build from the main branch (f6a4325):

image

I think it is useful to highlight the error message in some way (though I would prefer it was done in a consistent manner in all the dialogs e.g., Preferences dialog when invalid sketchbook location is selected, Firmware Updater dialog when process fails) to bring it to the attention of the user. However, if fixing the layout is more difficult, I am fine with the alternative fix of removing the highlight altogether as that would not be a regression.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: Fixed by 129436b

Deleting to trash fails

The file of the currently selected editor tab can be deleted by selecting "Delete" from the editor toolbar context menu. The IDE defaults to deleting this via the operating system's "trash" system in order to allow the user to recover the file.

🐛 The attempt to delete to "trash" fails and the user is forced to use the fallback of permanent deletion.

To reproduce

  1. Select "New Tab" from the editor toolbar context menu.
    The "Name for new file" dialog will open.
  2. Type Foo in the field of the "Name for new file" dialog.
  3. Click the "OK" button.
  4. Select "Delete" from the editor toolbar context menu.
    The "Move File to Trash" dialog will open.
  5. Click the "OK" button in the "Move File to Trash" dialog.

🐛 The attempt to delete the file fails:

Error deleting file

Failed to delete "Foo.ino" using the Trash. Do you want to permanently delete instead?

You can disable the use of Trash in the preferences.

Expected behavior

Deletion via the "trash" system works.

Arduino IDE version

090098d (tester build for e8a9ef7)

Operating system

Windows 11

Additional context

The following is printed to the logs when the deletion fails:

2023-07-30T12:08:32.365Z root ERROR Error deleting with trash: Error: Request 'delete' failed
    at Proxy.<anonymous> (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:3032271)
    at b.delete (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:3741400)
    at x.doDelete (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:3664211)
    at async x.delete (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:3663346)
    at async d.moveFileToTrash (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:8661047)
    at async Promise.all (index 1)
    at async d.delete (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:8660793)
    at async Promise.all (index 0)
    at async d.execute (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:8659279)
    at async g.executeCommand (file:///E:/incoming/review/2144/1-090098d/resources/app/lib/frontend/bundle.js:2:2954564)
Caused by: EntryNotFound (FileSystemError): Error: spawn E:\incoming\review\2144\1-090098d\resources\app\lib\backend\windows-trash.exe ENOENT
    at u (E:\incoming\review\2144\1-090098d\resources\app\lib\backend\main.js:2:1306074)
    at T.toFileSystemProviderError (E:\incoming\review\2144\1-090098d\resources\app\lib\backend\main.js:2:1329976)
    at T.delete (E:\incoming\review\2144\1-090098d\resources\app\lib\backend\main.js:2:1324688)
    at process.processTicksAndRejections (node:internal/process/task_queues:95:5)
    at async g.onRequest (E:\incoming\review\2144\1-090098d\resources\app\lib\backend\main.js:2:1064552)
    at async c.handleRequest (E:\incoming\review\2144\1-090098d\resources\app\lib\backend\main.js:2:1055445)

I used a secondary sketch file in the demo. If the primary sketch file is selected, the deletion fails silently.


Deletion to trash works as expected when using the IDE build from the main branch (f6a4325).

reverted DOM structure changes in single text input dialogs.
temporary workaround before aligning the validation style in all dialogs

Ref: eclipse-theia/theia#12585

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

I think it is useful to highlight the error message in some way

Thanks for spotting it! I agree, and I will take care of it in another PR. For now, I reverted (3529b14) the changes IDE2 pulled in from upstream:

Screenshot 2023-07-30 at 17 45 15

@kittaakos
Copy link
Contributor Author

kittaakos commented Jul 30, 2023

Deleting to trash fails

Thanks for the excellent review. See eclipse-theia/theia#12780. I am looking for a quick solution. Resolved via 129436b. ✅

@kittaakos
Copy link
Contributor Author

kittaakos commented Jul 31, 2023

IDE2 will lose the default value generated for the arduino.ide.updateChannel preference. It never worked. See #2157.

Akos Kitta added 2 commits July 31, 2023 10:22
revert packager script.

```
gyp info using [email protected] | darwin | x64
```

```
error [email protected]: The engine "node" is incompatible with this module. Expected version ">=16.14.0 <17". Got "18.16.1"
```
@kittaakos
Copy link
Contributor Author

"User data directory" name is arduino-ide

This should work now with the latest build from this PR. Resolved via e506b6f. I have verified it on macOS; the path was slightly different: /Users/<username>/Library/Application Support. Please double-check it on Windows. Thank you!

per1234
per1234 previously requested changes Aug 1, 2023
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: Fixed by 75480bd

"Date" field of "about" dialog uses fallback

The build date is shown in the "Date" field of the "about" dialog. The dialog code is written so that it falls back on using the value of "dev build" in this field if the build date data is not available.

🐛 This fallback value is being used in the field in the standard beta tester build generated by the repository's continuous development system.

To reproduce

  1. Download the beta tester build for the PR.
  2. Start the beta tester build.
  3. Select Help > About Arduino IDE from Arduino IDE menus.
    The "Arduino IDE" dialog will open.

🐛 The "Date" field is set to "dev build":

image

Expected behavior

The "Date" field is always set to the build timestamp when the build is generated by standard processes.

Arduino IDE version

3fc51f4 (tester build for 28bc470)

Operating system

Windows 11

Additional context

I haven't found the data from this field to be essential, so if there is some difficulty in populating it specifically for beta tester builds then I am OK with it being set to "dev build" for these builds. My concern is that this is an indicator that the system for populating the field is completely broken and it will be set to this fallback value even for the release builds, which would make the field useless and also possibly cause the users confusion.

merge in the frontend config and any other metadata from the final app's
package.json, as those are not available at theia build time.

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

I do not know what's been wrong with GitHub recently, but the commits do not appear on the PR.

Here is my commit (75480bd) on the top of the branch, but it's missing from the PR, and the build has not run either.

Screenshot 2023-08-01 at 20 38 30 Screenshot 2023-08-01 at 20 41 11

Anyway, if the build starts, please check the fixes. I verified it with a locally bundled app on macOS, and it worked. Thanks!

@per1234 per1234 dismissed their stale review August 2, 2023 07:21

All review items have been resolved.

Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

UPDATE: Fixed by 920a4fc

When I perform the instructions under "Build from source" in docs/development.md using a fresh clone of the repository checked out to this PR branch (f7cb8aa), step (4) fails:

Failed to start the electron application.
Error: Cannot find module './cli-protocol/cc/arduino/cli/settings/v1/settings_pb'
Require stack:
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/node/config-service-impl.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/node/sketches-service-impl.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/theia/electron-main-application.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/electron-arduino.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/arduino-electron-main-module.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/electron-app/src-gen/backend/electron-main.js
- /mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/node_modules/electron/dist/resources/default_app.asar/main.js
- 
    at Module._resolveFilename (node:internal/modules/cjs/loader:1002:15)
    at n._resolveFilename (node:electron/js2c/browser_init:2:109734)
    at Module._load (node:internal/modules/cjs/loader:848:27)
    at f._load (node:electron/js2c/asar_bundle:2:13330)
    at Module.require (node:internal/modules/cjs/loader:1068:19)
    at require (node:internal/modules/cjs/helpers:103:18)
    at Object.<anonymous> (/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/node/config-service-impl.js:50:23)
    at Module._compile (node:internal/modules/cjs/loader:1174:14)
    at Module._extensions..js (node:internal/modules/cjs/loader:1229:10)
    at Module.load (node:internal/modules/cjs/loader:1044:32) {
  code: 'MODULE_NOT_FOUND',
  requireStack: [
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/node/config-service-impl.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/node/sketches-service-impl.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/theia/electron-main-application.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/electron-arduino.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/arduino-ide-extension/lib/electron-main/arduino-electron-main-module.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/electron-app/src-gen/backend/electron-main.js',
    '/mnt/1c0bd221-e077-452c-b688-160dc700c122/git/arduino-ide_fresh-clone/node_modules/electron/dist/resources/default_app.asar/main.js',
    undefined
  ]
}

There is no arduino-ide-extension/lib/node/cli-protocol folder.

I don't have this problem when I run the same procedure in the repository where I had previously ran Arduino IDE from source with the repository checked out to the main branch.

docs/development.md Outdated Show resolved Hide resolved
docs/development.md Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
docs/development.md Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
kittaakos and others added 7 commits August 11, 2023 10:12
 - fixed: added missing build step
 - fixed: reordered sections: Windows tool must come before build part
 - added: prerequisites link to Theia and link to recommended tool: Code
 - added: how to clone repo and change dir to repo root
 - added: section how to start code + link to further docs

Signed-off-by: Akos Kitta <[email protected]>
@kittaakos
Copy link
Contributor Author

Thank you for the review! I have addressed the outstanding change requests and, most importantly, fixed the dev docs by adding the missing step. It should work now.

docs/development.md Outdated Show resolved Hide resolved
docs/development.md Outdated Show resolved Hide resolved
Copy link
Contributor

@per1234 per1234 left a comment

Choose a reason for hiding this comment

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

Thanks Akos!

@kittaakos kittaakos merged commit 9a6a457 into main Aug 14, 2023
19 checks passed
@kittaakos kittaakos deleted the app-packager branch August 14, 2023 10:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
topic: code Related to content of the project itself topic: infrastructure Related to project infrastructure topic: security Related to the protection of user data topic: theia Related to the Theia IDE framework type: enhancement Proposed improvement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants