-
Notifications
You must be signed in to change notification settings - Fork 79
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: Vendor cpp11 headers #451
base: main
Are you sure you want to change the base?
Conversation
Current Aviator status
This pull request is currently open (not queued). How to mergeTo merge this PR, comment See the real-time status of this PR on the Aviator webapp. Use the Aviator Chrome Extension to see the status of your PR within GitHub.
|
ok @krlmlr , it should look good now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, looks great!
I do wonder why cpp11::cpp_vendor()
insists on putting the headers into inst/include
.
|
||
try(dir.create(file.path(vendor_dir, "cpp11"), recursive = TRUE)) | ||
|
||
for (f in finp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What is finp
?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@krlmlr sorry, I accidentally deleted that part, it is a list of all the headers I added. I will re-add that line
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to run this script on my machine without errors (and perhaps without unnecessary actions).
} | ||
|
||
unlink(file.path(vendor_dir, "inst"), recursive = TRUE) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Everything below here seems to be a one-time action?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
yes
Co-authored-by: Kirill Müller <[email protected]>
I will send then a PR, because it is the default, which forced me to write some code for a 1 time action (otherwise I had to create a video of my clicks) |
Thanks. I don't think it's a one-time action, at least in the medium term. If cpp11 gets nice new features, we want to use them. |
Yes!
I mean, the script can be re-run many times. For now, it is a one time action, as we do not need to run it every time a commit is made
Get Outlook for Android<https://aka.ms/AAb9ysg>
…________________________________
From: Kirill Müller ***@***.***>
Sent: Thursday, November 9, 2023 12:17:01 AM
To: r-dbi/RPostgres ***@***.***>
Cc: Mauricio Andres Vargas Sepulveda ***@***.***>; Author ***@***.***>
Subject: Re: [r-dbi/RPostgres] chore: Vendor cpp11 headers (PR #451)
Thanks. I don't think it's a one-time action, at least in the medium term. If cpp11 gets nice new features, we want to use them.
—
Reply to this email directly, view it on GitHub<#451 (comment)>, or unsubscribe<https://github.com/notifications/unsubscribe-auth/ACM7UOJVWX4X64FEF7R5BOTYDRRM3AVCNFSM6AAAAAA7DJZF3CVHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMYTQMBTGE3TMNZRGA>.
You are receiving this because you authored the thread.Message ID: ***@***.***>
|
@krlmlr hi, do you think this can be merged? |
|
||
try(dir.create(file.path(vendor_dir, "cpp11"), recursive = TRUE)) | ||
|
||
for (f in finp) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'll need to run this script on my machine without errors (and perhaps without unnecessary actions).
Co-authored-by: Kirill Müller <[email protected]>
@krlmlr any update on this? |
Let's wait for r-lib/cpp11#353. Also, the upside of no vendoring means that there's less maintenance effort at the (fairly low) risk of breakages from upstream. |
@krlmlr here are my changes in relation to #450
in the issue I detail where to find my changes and how I vendored the headers
Closes #450.