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

fix: Update flake to fix failing build #830

Merged
merged 4 commits into from
Feb 23, 2024
Merged

Conversation

detroyejr
Copy link
Contributor

I've been meaning to try out polars for a while and was pleased to see a flake available. Things didn't build on first run, so I made a few updates to get this working again.

  1. nix flake update.
  2. cargo update -p ahash - This was the main reason for the build failure. Some recent changes to ahash seems to have broken this and several other libraries. I left all other dependencies alone.
  3. Add a patchPhase with patchShebangs ./configure.

One question: in the current main branch, polars is not included as a package dependency in the custom renv. A downstream version of this repo did include it but it was removed at some point. I'm wondering if that was omitted on purpose or if that should be added back like so:

          packages = with pkgs.rPackages; [ devtools languageserver renv rextendr rpolars ];

Without it, I cannot run nix develop github:pola-rs/r-polars and get a working version of R with polars available.

Tested with nix develop github:detroyejr/r-polars. Once everything builds R -e 'packageVersion("polars")' returns 0.14.0.9000. The example from the README also works.

@eitsupi
Copy link
Collaborator

eitsupi commented Feb 22, 2024

Thank you for your contribution. renv is not currently in use and can be removed.

@Sicheng-Pan Could you take a look at this?

@Sicheng-Pan
Copy link
Collaborator

Thanks a lot for the patch! The reason I did not add rpolars as a dependency in the devshell is because (from my understanding) Nix does not cache the intermediate build cache for Rust, so if made changes to the code I need to re-compile from scratch, which is really painful for me when I develop. So for simplicity I choose to use devtools in R to compile the code in the devshell, and it will automatically load the library when it finishes. devtools also helps to build the docs and perform sanity checks, and I'm unsure if it can be replaced by some Nix script. If devtools are no longer needed then I believe that the "proper" approach could be using something like crane to speed up the compilation a bit, and in that case we could put rpolars as a dependency in the devshell.

@Sicheng-Pan
Copy link
Collaborator

@detroyejr Also it's been a while since I last updated the flake lock, so feel free to update it there is no conflict!

@detroyejr
Copy link
Contributor Author

That makes sense. I've removed that from the renv.

The flake lock is updated and I didn't see any issues.

@eitsupi
Copy link
Collaborator

eitsupi commented Feb 22, 2024

The CI failure of macos-14 is came from the image update actions/runner-images#9394

@Sicheng-Pan
Copy link
Collaborator

@eitsupi Just to confirm the renv R package is no longer a dependency for the project right? If that is the case it could also be removed from the flake.

@eitsupi
Copy link
Collaborator

eitsupi commented Feb 23, 2024

Just to confirm the renv R package is no longer a dependency for the project right?

Yes, see #586
I removed it because it looked like no one was using it.

@detroyejr
Copy link
Contributor Author

Got it. Removed the renv package from the flake as well.

Sicheng-Pan

This comment was marked as outdated.

@eitsupi
Copy link
Collaborator

eitsupi commented Feb 23, 2024

Could you please pull in the main branch?
#832 should fix the CI failure.

Copy link
Collaborator

@Sicheng-Pan Sicheng-Pan left a comment

Choose a reason for hiding this comment

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

Thanks for the help!

@eitsupi
Copy link
Collaborator

eitsupi commented Feb 23, 2024

Thanks, we are freezing the Rust side in preparation for the 0.14.1 release and will merge it after the 0.14.1 release.

@eitsupi eitsupi marked this pull request as draft February 23, 2024 04:27
@eitsupi eitsupi marked this pull request as ready for review February 23, 2024 13:47
@eitsupi eitsupi merged commit 73462dc into pola-rs:main Feb 23, 2024
34 checks passed
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.

3 participants