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.

Sign up for GitHub

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

Jump to bottom

Fixing Electron runner #5633

Merged
merged 63 commits into from
Feb 19, 2023
Merged

Fixing Electron runner #5633

merged 63 commits into from
Feb 19, 2023

Conversation

wdanilo
Copy link
Member

@wdanilo wdanilo commented Feb 11, 2023 •
edited
Loading

Pull Request Description

This PR improves the Electron implementation:

  • It is now written in TypeScript, not JS.
  • Electron performance options are used right now.
  • Options are shared between TS and Rust code in an uniform way.
  • Options are cleaned.
  • Electron is updated to the newest version which prevents it from crashing on MacOS M2.

Checklist

Please include the following checklist in your PR:

  • The documentation has been updated if necessary.
  • All code conforms to the
    Scala,
    Java,
    and
    Rust
    style guides.
  • All code has been tested:
    • Unit tests have been written where possible.
    • If GUI codebase was changed: Enso GUI was tested when built using BOTH
      ./run ide build and ./run ide watch.

@CLAassistant
Copy link

CLAassistant commented Feb 11, 2023 •
edited
Loading

CLA assistant check
All committers have signed the CLA.

kazcw
kazcw reviewed Feb 17, 2023
View reviewed changes
lib/rust/json-to-struct/src/lib.rs Outdated

/// Parse the input token stream as file paths.
fn files_paths(input: proc_macro::TokenStream) -> Vec<PathBuf> {
let mut paths: Vec<String> = vec![];
Copy link
Contributor

Choose a reason for hiding this comment

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

Written but never read?

kazcw
kazcw reviewed Feb 17, 2023
View reviewed changes
app/ide-desktop/lib/client/src/config.ts Outdated
description:
`Record a performance profile and save it to a file. To view the ` +
`results, use the 'profiling-run-graph' entry point, such as ` +
`'enso -startup.entry-point=profiling-run-graph -profile.load-workflow-profile=profile.json'.`,
Copy link
Contributor

@kazcw kazcw Feb 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

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

startup.entry-point and startup.load-workflow-profile have both been renamed

kazcw
kazcw reviewed Feb 17, 2023
View reviewed changes
app/ide-desktop/lib/client/src/config.ts Outdated
passToWebApplication: false,
default: true,
description:
'Force using discrete GPU when there are multiple GPUs available',
Copy link
Contributor

Choose a reason for hiding this comment

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

Missing .

@kazcw
Copy link
Contributor

kazcw commented Feb 17, 2023

-profile.test-workflow shouldn't exist--it's the same thing as profile.workflow. The previous code was inconsistent about what the option was called.

@kazcw
Copy link
Contributor

kazcw commented Feb 17, 2023

Most options use kebab-case, but some use snake_case: chrome.force_low_power_gpu, chrome.force_high_performance_gpu.

@kazcw
Copy link
Contributor

kazcw commented Feb 17, 2023

Some invalid arguments are silently ignored, e.g. if I pass -loader.download-to-init-ratio=NotANumber I don't see any warning.

kazcw
kazcw reviewed Feb 17, 2023
View reviewed changes
app/ide-desktop/lib/content-config/src/config.json
},
"skipMinVersionCheck": {
"value": "EVAL(Version.isDev())",
"defaultDescription": "true in local builds, false otherwise.",
Copy link
Contributor

Choose a reason for hiding this comment

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

Should not end in .

@kazcw
Copy link
Contributor

kazcw commented Feb 17, 2023

QA ✔️

@mwu-tow mwu-tow added the CI: Clean build required CI runners will be cleaned before and after this PR is built. label Feb 17, 2023
@wdanilo
Copy link
Member Author

wdanilo commented Feb 18, 2023

-loader.download-to-init-ratio=NotANumber

It is logged in console. You need to expand the a log tab to see it, unfortunately though:
image

@wdanilo wdanilo merged commit 663ed1e into develop Feb 19, 2023
@wdanilo wdanilo deleted the wip/wdanilo/shader-compilation-improvement-184304289 branch February 19, 2023 00:38
farmaazon added a commit that referenced this pull request Feb 23, 2023
@farmaazon
Revert "Fixing Electron runner (#5633)"
8c760f8 
This reverts commit 663ed1e.
@Az20033333 Az20033333 mentioned this pull request Oct 6, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kazcw kazcw kazcw approved these changes

@mwu-tow mwu-tow mwu-tow left review comments

@farmaazon farmaazon farmaazon left review comments

@MichaelMauderer MichaelMauderer Awaiting requested review from MichaelMauderer

Assignees
No one assigned
Labels
CI: Clean build required CI runners will be cleaned before and after this PR is built. CI: No changelog needed Do not require a changelog entry for this PR.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@wdanilo @CLAassistant @kazcw @mwu-tow @farmaazon