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

Add electron build workflow #100

Merged
merged 10 commits into from
Jul 15, 2024
Merged

Add electron build workflow #100

merged 10 commits into from
Jul 15, 2024

Conversation

azmy60
Copy link
Contributor

@azmy60 azmy60 commented Jun 18, 2024

In attempt to support electron build as mentioned in #70.

This PR aims to build binaries for electron 18.3.x to latest.

carlopi

This comment was marked as outdated.

@carlopi
Copy link
Collaborator

carlopi commented Jul 13, 2024

Thanks for the contribution, could you have a look at why CI is failing?

@azmy60
Copy link
Contributor Author

azmy60 commented Jul 15, 2024

@carlopi So it was failing because I needed to add the patch version to the electron-versions.json. It should still generate binaries with the <major>.<minor> version, so 18.3.9 will generate duckdb-v1.0.1-dev14.0-electron-v18.3-linux-x64.tar.gz.

@carlopi carlopi self-requested a review July 15, 2024 07:07
Copy link
Collaborator

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks, looks good, last request, could you make it so only a small subset (say up to 3?) of the electron builds are actually run on pull requests?

I could also take a look at tweaking this at some later point, but I think having like the setup step be conditional, and either use the full matrix or only selected rows it would be handy to have, since otherwise this will become very heavy.

Copy link
Collaborator

@carlopi carlopi left a comment

Choose a reason for hiding this comment

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

Thanks!!

@azmy60
Copy link
Contributor Author

azmy60 commented Jul 15, 2024

No problem! I just filtered 2 versions that we're currently using in our repo now. :)

@carlopi carlopi merged commit 13104d4 into duckdb:main Jul 15, 2024
22 of 24 checks passed
@carlopi
Copy link
Collaborator

carlopi commented Jul 15, 2024

@azmy60: given you have it still fresh, could you take a look at the syntax that triggers this problem https://github.com/duckdb/duckdb-node/actions/runs/9937704658:
  Error when evaluating 'strategy' for job 'osx-electron-x64'. .github/workflows/Electron.yml (Line: 167, Col: 18): Error parsing fromJson,.github/workflows/Electron.yml (Line: 167, Col: 18): Error reading JToken from JsonReader. Path '', line 0, position 0.,.github/workflows/Electron.yml (Line: 167, Col: 18): Unexpected value ''Show less  

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