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

ci: build prqlc on PRs #2568

Merged
merged 14 commits into from
May 15, 2023
Merged

ci: build prqlc on PRs #2568

merged 14 commits into from
May 15, 2023

Conversation

eitsupi
Copy link
Member

@eitsupi eitsupi commented May 14, 2023

Addresses #1910

@eitsupi
Copy link
Member Author

eitsupi commented May 14, 2023

@eitsupi eitsupi force-pushed the upload-prqlc branch 2 times, most recently from fa3dcb2 to 21a1cc2 Compare May 14, 2023 10:58
@eitsupi

This comment was marked as outdated.

@eitsupi eitsupi marked this pull request as ready for review May 14, 2023 12:10
@eitsupi eitsupi requested a review from max-sixty May 14, 2023 14:10
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Excellent!! Thanks a lot.

A couple of points:

  • No stress about Linux arm64. I see some comments in Cannot cross-compile aarch64-musl on x86_64 linux rust-lang/rust#46651 (comment), not sure if they would help, but fine to leave for another day.

    • I've also heard about https://github.com/cross-rs/cross, but don't have any experience with it. To the extent this can encapsulate much of the complication around non-default targets, that could work well. I don't see obvious docs for how to use with GHA — not sure whether that's because it's so easy or so difficult! :)
  • I'm not sure whether this necessarily needs to be an "action" rather than a normal workflow, especially now that GH has loosened the restrictions on workflows calling other workflows. But it's also fine as an action.

  • Do you know what we'd have to do to get these publishing on each release? We may have to auto-release IIUC. That would be a nice use of this.

.github/actions/build-prqlc/action.yaml Show resolved Hide resolved
.github/actions/build-prqlc/action.yaml Outdated Show resolved Hide resolved
.github/actions/build-prqlc/action.yaml Outdated Show resolved Hide resolved
@@ -108,3 +108,27 @@ jobs:
allowed-failures: check-links
allowed-skips: test-all, publish-web, nightly
jobs: ${{ toJSON(needs) }}

build-prqlc:
Copy link
Member

Choose a reason for hiding this comment

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

Before merging, I think probably we have this in test-all rather than pull-request — or are there times it would fail on new code that we'd want to know about?

Copy link
Member Author

Choose a reason for hiding this comment

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

My idea was to provide an easy way to try out features added by pull requests.
For example, I believe duckdb builds and uploads duckdb CLI executables with every pull request.

Copy link
Member

Choose a reason for hiding this comment

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

OK great, we can try it.

I do notice our CI queue filling up occasionally, and this would almost double the number of jobs. So if it becomes an issue we could always add a pr-build-artifacts label and run it based on that.

Definitely fine to try, I agree that would be a nice thing to have access to.

.github/actions/build-prqlc/action.yaml Outdated Show resolved Hide resolved
eitsupi and others added 3 commits May 15, 2023 04:20
Co-authored-by: Maximilian Roos <5635139+max-sixty@users.noreply.github.com>
@eitsupi
Copy link
Member Author

eitsupi commented May 15, 2023

Thank you for reviewing this.

I considered cross, but did not try it at this time due to the need for a completely different workflow to use Docker and concerns about slowdowns due to QEMU emulation.

Attaching these to the release would be easy as it is just one (?) additional step.

@eitsupi
Copy link
Member Author

eitsupi commented May 15, 2023

Thanks to the advice I received in rust-lang/stacker#80, I am now able to build for Linux arm64.
(I have confirmed that the prqlc binary works on my Raspberry Pi 4)

image

Once this PR is merged I will work on attaching these to the releases.

@eitsupi eitsupi requested a review from max-sixty May 15, 2023 15:44
Copy link
Member

@max-sixty max-sixty left a comment

Choose a reason for hiding this comment

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

Looks great!

I see there's an unrelated failure — don't let that delay merging this, I'll have a look at that later unless anyone else gets to it

@eitsupi eitsupi merged commit 0f04b2e into PRQL:main May 15, 2023
@eitsupi eitsupi deleted the upload-prqlc branch May 15, 2023 22:41
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