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

chore: add configure scripts and update CI #435

Merged
merged 40 commits into from
Oct 28, 2023
Merged

chore: add configure scripts and update CI #435

merged 40 commits into from
Oct 28, 2023

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Oct 20, 2023

Part of #408, close #431

  • Move some logics on Makevars to the configure script
  • Rewrite CI workflows
  • Drop R 4.1.x support (for not to use old R on Windows)

Port the configure scripts and GitHub Actions workflows from https://github.com/eitsupi/prqlr.

@eitsupi eitsupi force-pushed the configure branch 2 times, most recently from e5454dd to d595159 Compare October 21, 2023 11:22
@eitsupi eitsupi changed the title wip chore: add configure scripts [skip ci] wip chore: add configure scripts Oct 21, 2023
@eitsupi eitsupi mentioned this pull request Oct 26, 2023
@eitsupi eitsupi changed the title wip chore: add configure scripts chore: add configure scripts Oct 28, 2023
@eitsupi eitsupi marked this pull request as draft October 28, 2023 14:21
@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 28, 2023

The release workflow seems to be building the library twice. It needs to be fixed.

@eitsupi eitsupi marked this pull request as ready for review October 28, 2023 14:42
@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 28, 2023

Merging this PR should reduce CI execution time due to Rust caching.
I would like to do a major rewrite of the CI after this is merged.

Copy link
Collaborator

@etiennebacher etiennebacher left a comment

Choose a reason for hiding this comment

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

I don't know much in bash so I can't properly review this but reducing CI time would be great. It looks like it still takes about 20min in this PR, is it because it has to rebuild everything before caching is used?

Also, I didn't understand why we bumped the required R version to 4.2. I'd rather keep it 4.1 if there are no reasons other than for CI.

.github/actions/setup/action.yaml Show resolved Hide resolved
.github/actions/setup/action.yaml Outdated Show resolved Hide resolved
@eitsupi
Copy link
Collaborator Author

eitsupi commented Oct 28, 2023

It looks like it still takes about 20min in this PR, is it because it has to rebuild everything before caching is used?

I think that's why it hasn't been saved yet because the cache is only saved from the main branch.
Once this is merged and run once, the build time should be significantly faster as the binaries are cached.

Also, I didn't understand why we bumped the required R version to 4.2. I'd rather keep it 4.1 if there are no reasons other than for CI.

To support R 4.1, the Makevars file for Windows must be split into Makevars.win and Makevars.ucrt.
Let's stop supporting R 4.1 because the cost of managing two files is high.

Also, the latest version of libR-sys no longer supports R 4.1 (extendr/libR-sys#170), so it will be necessary to end support for R 4.1 in the near future.

eitsupi and others added 2 commits October 28, 2023 16:21
Co-authored-by: Etienne Bacher <52219252+etiennebacher@users.noreply.github.com>
@eitsupi eitsupi changed the title chore: add configure scripts chore: add configure scripts and update CI Oct 28, 2023
@eitsupi eitsupi merged commit 3590181 into main Oct 28, 2023
17 checks passed
@eitsupi eitsupi deleted the configure branch October 28, 2023 21:31
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.

Rust cache does not seem working on Windows
2 participants