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

Publish the library files as an artifact after builds #3953

Closed
Seddryck opened this issue Dec 17, 2023 · 9 comments · Fixed by #4075
Closed

Publish the library files as an artifact after builds #3953

Seddryck opened this issue Dec 17, 2023 · 9 comments · Fixed by #4075
Labels

Comments

@Seddryck
Copy link

What's up?

Regarding ongoing discussions on Discord on the pros/cons of having a mono-repo vs a repo dedicated for each language binding, we came to a point where the lack of proper packaging of the library files (.dll, .dylib and .so) is a major impediment when executing the "repo by language bindings" strategy.

This issue is to track progress on packaging library files during builds and make them available for other repos.

@max-sixty max-sixty changed the title Publish the library files as an artefact after builds Publish the library files as an artifact after builds Dec 17, 2023
@max-sixty
Copy link
Member

@vanillajonathan if you'd be interested, I think this would be quite helpful. The goal would be to have those files published on each release in the release assets (e.g. at the bottom here: https://github.com/PRQL/prql/releases/tag/0.10.1)

@vanillajonathan
Copy link
Collaborator

We use our build-prql GitHub Action which generates an artifact with prqlc, the license and a readme.
Should we create a new GitHub Action called build-libprql based on the build-prql action?
How does this work with caching? and we would run sudo apt-get update in both actions?

@max-sixty
Copy link
Member

Great questions — thanks for thinking through it.

I think two main options:

  • Copy paste the build-prqlc action, and adjust it for libprql. This is probably the easiest way, even though the result won't be quite as elegant, because there will be duplication.
  • Attempt to add steps to the build-prqlc action. This might work very well, but I also worry that global configs such as RUSTFLAGS=-Ctarget-feature=+crt-static might get in the way if we don't want those for libprql

(Possibly there's a third option of "split up the parts that are common across both", but my guess is that will be much easier after making it work with one of those)

Your call on what to try. If you're not sure, I might try and get it working with the copy-paste, and then if you find you don't need to change anything, integrate them into a single action.

Then I think we'll have to add it to this so it publishes on release.

How does this work with caching?

The one learning on caching is here — it's better to build for everything rather than only libprql, because the CI caches are stored for everything, and building a single package can use a different set of features.

and we would run sudo apt-get update in both actions?

@eitsupi would you know whether we should be building this using musl? Or is that only for binaries?

(if we don't use musl, I don't think we need to apt-get install anything)

@eitsupi
Copy link
Member

eitsupi commented Dec 18, 2023

would you know whether we should be building this using musl?

Since both gnu ABI and musl ABI may be used, don't we need to build both?

I have confirmed that prqlr works with prql-compiler built with only musl, whether R is gnu or musl, so I only use musl for binary builds, but I am not sure if this is true in all cases.
https://github.com/eitsupi/prqlr/blob/66949df677a1b3b5e971e2cc2aa5c927fa063417/.github/workflows/release-lib.yml#L34-L44

@eitsupi
Copy link
Member

eitsupi commented Dec 18, 2023

When building using the gnu target, we can build binaries that can be used by many clients by lowering the Ubuntu version of the runner of CI as much as possible, but this will not work for distributions that only use musl, such as Alpine Linux.

If we build using the musl target, it may not work if the program being linked uses the gnu ABI.
(For CLI, only musl is generally sufficient as it is never linked)

@max-sixty
Copy link
Member

Thanks a lot @eitsupi , that makes sense.

Given the immediate need is for dotnet, I think we could consider only having for musl in Linux if that's easier than doing this for both gnu & musl, and then implement gnu if someone specifically needs it...

@eitsupi
Copy link
Member

eitsupi commented Dec 20, 2023

I think it makes sense to update the existing action and make the target name an argument so that we can build something other than prqlc.

@Seddryck
Copy link
Author

Hi @max-sixty, will take a look to the next steps to build .NET bindings probably tomorrow.

@max-sixty
Copy link
Member

I think it makes sense to update the existing action and make the target name an argument so that we can build something other than prqlc.

FYI @eitsupi I looked at doing this — but there were too many differences, such as the path of the package. I started a conversation to try and align names to make things a bit more similar, but there's enough that's different that copy&paste&edit was easier

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants