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

Modifying configs for linux packages #1664

Merged
merged 7 commits into from
Mar 25, 2025
Merged

Conversation

rsamf
Copy link
Contributor

@rsamf rsamf commented Mar 23, 2025

Description

Builds Linux (debian) and AppImages.

Related Issues

Contributes to #229 by packaging onlook for Linux systems

Type of Change

  • Bug fix
  • New feature
  • Documentation update
  • Release
  • Refactor
  • Other (please describe):

Testing

  • I tested both Debian x64 and AppImage x64 packages on Ubuntu 22 by installing them, running them, and viewing an existing project. It seemed good to me.
  • I could not test arm installations.

Screenshots (if applicable)

Additional Notes

  • I had to set icon to 'build/icon.icns' because electron-builder wasn't finding the correct icon for some reason, and if I set it to 'build/icon.png', that also didn't work.
  • I also had to set executableName because electron-builder was setting it to "@onlookstudio" and not "Onlook".
  • Desktop.Name was seemingly redundant. The Onlook.desktop file has Name=Onlook without it.

Important

Add Linux build support for Debian and AppImage in Electron Builder and update GitHub Actions workflow.

  • Build Configuration:
    • Add Linux (AppImage, deb) targets in base.ts.
    • Set executableName to Onlook and icon to build/icon.icns in base.ts.
  • GitHub Actions:
    • Add linux-latest to build matrix in build.yml.
  • Scripts:
    • Update package script in package.json to use --config builder-config/base.ts.

This description was created by Ellipsis for 63f009a. It will automatically update as commits are pushed.

Copy link

@ellipsis-dev ellipsis-dev bot left a comment

Choose a reason for hiding this comment

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

👍 Looks good to me! Reviewed everything up to 63f009a in 1 minute and 11 seconds

More details
  • Looked at 41 lines of code in 3 files
  • Skipped 0 files when reviewing.
  • Skipped posting 7 drafted comments based on config settings.
1. .github/workflows/build.yml:32
  • Draft comment:
    Ensure inclusion of 'linux-latest' in build matrix is intended for Linux packaging.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
2. apps/studio/builder-config/base.ts:68
  • Draft comment:
    Using 'build/icon.icns' for Linux packaging may lead to icon format issues; consider verifying if a Linux-friendly format (e.g., PNG) is acceptable or document this limitation.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
3. apps/studio/package.json:28
  • Draft comment:
    Explicitly specifying '--config builder-config/base.ts' in the package script clarifies the build configuration; this change appears deliberate and helpful.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
4. .github/workflows/build.yml:32
  • Draft comment:
    Added linux-latest to the build matrix. Ensure the Linux runner provides the required environment for packaging.
  • Reason this comment was not posted:
    Confidence changes required: 0% <= threshold 50%
    None
5. apps/studio/builder-config/base.ts:68
  • Draft comment:
    Linux config: 'executableName' is set and the icon is now 'build/icon.icns'. Verify that an ICNS icon works as intended on Linux; consider adding a comment explaining this workaround.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
6. apps/studio/package.json:28
  • Draft comment:
    The package script now explicitly passes '--config builder-config/base.ts', ensuring consistent build config usage. Note: The PR description mentions onboarding docs, but the changes focus on Linux packaging—please confirm the issue reference is correct.
  • Reason this comment was not posted:
    Confidence changes required: 33% <= threshold 50%
    None
7. apps/studio/package.json:30
  • Draft comment:
    The script name 'pree2e' appears to be a potential typographical error. Please double-check if this is the intended script name or if it should be corrected (e.g., to 'pre-e2e' or another consistent naming).
  • Reason this comment was not posted:
    Comment was on unchanged code.

Workflow ID: wflow_7oNPrxhvm8yuOXqM


You can customize Ellipsis with 👍 / 👎 feedback, review rules, user-specific overrides, quiet mode, and more.

@rsamf
Copy link
Contributor Author

rsamf commented Mar 23, 2025

The 2 extra commits are there because I had some personal modifications in there that I didn't explicitly stage with git add but it seemed like every time I did a git commit there was a hook to put everything that was unstaged in 😆

@Kitenite
Copy link
Contributor

Yeah that's precommit hooks to format the code haha. You can run commit with --no-verify next time.

@Kitenite
Copy link
Contributor

See actions here: https://github.com/onlook-dev/onlook/actions/runs/14025015625

@Kitenite
Copy link
Contributor

Will update you with the draft release when it's available

@rsamf
Copy link
Contributor Author

rsamf commented Mar 24, 2025

@Kitenite Kitenite merged commit 26124f5 into onlook-dev:main Mar 25, 2025
zongkelong pushed a commit to zongkelong/coolook that referenced this pull request Mar 27, 2025
…slation_zh

* 'main' of https://github.com/onlook-dev/onlook:
  Add brand tests (onlook-dev#1675)
  Disable elide lines for precommit hooks (onlook-dev#1674)
  App Panel, Installed Apps, App Details (onlook-dev#1608)
  Modifying configs for linux packages (onlook-dev#1664)
  fix: prevent duplicate variable in tailwind (onlook-dev#1671)
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.

2 participants