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

refactor!: simplify Makevars files and rename env vars for building the library #693

Merged
merged 12 commits into from
Jan 20, 2024

Conversation

eitsupi
Copy link
Collaborator

@eitsupi eitsupi commented Jan 13, 2024

Related to #80, #241, #300, and #445

  • Allows to select any feature via LIBR_POLARS_FEATURES instead of RPOLARS_FULL_FEATURES="true". This allows users to freely select features.
  • Unify environment variable names from RPOLARS* to LIBR_POLARS* for consistency.
  • Delete parts that are no longer needed.

These changes are brought over from prqlr.

@eitsupi eitsupi added this to the 1st CRAN Release milestone Jan 13, 2024
@eitsupi eitsupi changed the title refactor!: simplify Makevars files refactor!: simplify Makevars files and rename env vars for building the library Jan 13, 2024
@eitsupi eitsupi marked this pull request as ready for review January 13, 2024 04:19
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.

Small nitpick but shouldn't these be named LIB_R_POLARS_* instead?

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 13, 2024

shouldn't these be named LIB_R_POLARS_* instead?

Good point.
The names are a very difficult issue, but I believe that libr_polars is the name of the actual binary library that will be generated, and adding an underscore like LIB_R_POLARS would make it confusing.

LIBNAME = libr_polars.a

LIB_NAME: libr_polars

@etiennebacher
Copy link
Collaborator

I believe that libr_polars is the name of the actual binary library that will be generated

Would it be a breaking change to rename this binary lib_r_polars? It's just weird to have libr/LIBR as prefix. Since renaming envvars is a breaking change, we might as well rename the binary at the same time.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 13, 2024

Would it be a breaking change to rename this binary lib_r_polars? It's just weird to have libr/LIBR as prefix.

This name is automatically generated from the crate name "r-polars". i.e. "lib" + "r-polars" -> "libr-polars" -> "libr_polars"
This is the cargo's default behavior, so I don't think there's anything strange about it.

@etiennebacher
Copy link
Collaborator

Sorry for being nitpicky on this, but I'd prefer envvars to be named R_POLARS_*, e.g R_POLARS_FEATURES. This doesn't match the binary name but from the user perspective it makes more sense IMO. The users never interact directly with the binary generated by cargo, so for them there's no consistency issue in having the binary named libr_polars and the envvars named R_POLARS_*.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 13, 2024

Sorry for being nitpicky on this, but I'd prefer envvars to be named R_POLARS_*, e.g R_POLARS_FEATURES. This doesn't match the binary name but from the user perspective it makes more sense IMO. The users never interact directly with the binary generated by cargo, so for them there's no consistency issue in having the binary named libr_polars and the envvars named R_POLARS_*.

That's a good point to consider, and I was thinking the same way when I opened this PR.

However, we can separate environment variables related to Rust build from other environment variables (for example, environment variables that affect the behavior of r-polars may be added) by using "LIB" as a prefix.
I thought maybe it wasn't a bad idea.

@etiennebacher
Copy link
Collaborator

However, we can separate environment variables related to Rust build from other environment variables (for example, environment variables that affect the behavior of r-polars may be added) by using "LIB" as a prefix.

Right, that's a good distinction to make, thanks for explaining.

At some point, we should add a vignette listing all possible envvars, both for the Rust build and for the use in R (in addition to what you put in the "installation" vignette).

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 13, 2024

Thanks for your review!
I would like @sorhawell to confirm this as well, so I will merge it afterwards.

@sorhawell
Copy link
Collaborator

Thanks for your review! I would like @sorhawell to confirm this as well, so I will merge it afterwards.

Hi @eitsupi and @etiennebacher .

You probably noticed the frequency and quality of PRs and reviews have gone down lately. I think so. I will take a pause from open source for half year or more I think. I want to try out some new directions in a life. It has been a great honor to work with you. You can replace me as maintainer and/or "first author" whenever it makes sense.

I will try to commit the Rstudio completion PR and give up on the vscode completion PR.

@eitsupi
Copy link
Collaborator Author

eitsupi commented Jan 20, 2024

@sorhawell Thanks for the reply. Glad to hear you are doing well.

I will merge this and move on.

@eitsupi eitsupi merged commit bc675ca into main Jan 20, 2024
34 checks passed
@eitsupi eitsupi deleted the rewrite-makevars branch January 20, 2024 02:11
@etiennebacher
Copy link
Collaborator

@sorhawell I hope this break will be good for you and you'll find things you enjoy doing. I'm always impressed by the amount of work you put to make this package working, thanks so much

@grantmcdermott
Copy link
Collaborator

+1 OSS work is very rewarding... but it can also be incredibly draining. @sorhawell I'm among the many grateful people for all the work that you've put into r-polars. (Equally grateful for @eitsupi and @etiennebacher picking up the maintenance reins.) Enjoy the break and I hope that it brings you a sense of rest and rejuvenation.

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.

None yet

4 participants